Add operators base #713
Add operators base #713
Conversation
WalkthroughAdds operator-based atomic update support: new Operator class and validator, adapter capability flags, operator-aware SQL generation/binding across adapters, Database integration for extraction/validation/refetch, benchmarking CLI, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant DB as Database
participant OP as Operator.parse / extract
participant VAL as OperatorValidator
participant Adapter as SQL Adapter
participant SQL as RDBMS
User->>DB: updateDocument(collection, payload)
DB->>OP: extractOperators(payload)
DB->>VAL: validateOperators(collection, operators)
VAL-->>DB: valid / throws
DB->>Adapter: updateDocument(regularAttrs, operators)
Adapter->>Adapter: split regular vs operator attrs
Adapter->>Adapter: build SQL (include getOperatorSQL fragments)
Adapter->>Adapter: bind regular params
Adapter->>Adapter: bindOperatorParams(bindIndex)
Adapter->>SQL: execute UPDATE/UPSERT with operator expressions
SQL-->>Adapter: result
Adapter-->>DB: success
DB->>DB: refetchDocuments(if operators present)
DB-->>User: updated document (with computed values)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
Apply same sign-aware overflow/underflow checks as Postgres and MariaDB
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
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
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
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.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
src/Database/Validator/Operator.php (3)
124-128: Use strict zero-check for divide/modulo.Prevent coercion bugs by using strict comparison.
- 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: Match arms: use strict inequality for zero.Avoid type juggling; mirror the validation.
- 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,
238-247: Validate operator values for arrayIntersect/arrayDiff.Require non-empty array input; prevents adapter-time errors.
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])) { + $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])) { + $this->message = "{$method} operator requires a non-empty array value"; + return false; + }Also applies to: 251-256
src/Database/Adapter/SQL.php (3)
1977-1981: ARRAY_REMOVE: preserve original when value not found.Avoid setting column to NULL when JSON_SEARCH returns NULL.
- 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} + )";
1985-1991: ARRAY_INSERT: insert typed JSON, not a quoted string.Use JSON_EXTRACT on the bound value.
- return "{$quotedColumn} = JSON_INSERT({$quotedColumn}, CONCAT('$[', :$indexKey, ']'), :$valueKey)"; + return "{$quotedColumn} = JSON_INSERT({$quotedColumn}, CONCAT('$[', :$indexKey, ']'), JSON_EXTRACT(:$valueKey, '$'))";
2021-2033: ARRAY_FILTER: compare JSON values and support aliases.Use JSON_EXTRACT and cover allowed condition names; otherwise filter may be a no‑op.
- 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 ENDsrc/Database/Adapter/SQLite.php (1)
1398-1431: Critical: Remove duplicate bindIndex increment in ARRAY_FILTER binding.The
ARRAY_FILTERoperator increments$bindIndexin two places:
- In
getOperatorSQL()at line 1702 during SQL generation- In
bindOperatorParams()at line 1423 during parameter bindingSince both methods share the same
$bindIndexreference, this causes the bind-index sequence to skip a number, leading to placeholder/parameter misalignment.Solution: Remove the increment at line 1423. The increment in
getOperatorSQL()(line 1702) is sufficient and keeps SQL generation and binding synchronized.Apply this diff:
$comparisonTypes = ['equal', 'notEqual', 'greaterThan', 'greaterThanEqual', 'lessThan', 'lessThanEqual']; if (in_array($filterType, $comparisonTypes)) { $bindKey = "op_{$bindIndex}"; $value = (is_bool($filterValue)) ? (int)$filterValue : $filterValue; $stmt->bindValue(":{$bindKey}", $value, $this->getPDOType($value)); - $bindIndex++; }Based on past review comments.
src/Database/Adapter/MariaDB.php (1)
2048-2066: Critical: Add missing comparison operators in ARRAY_FILTER.The
ARRAY_FILTERCASE statement only handlesgreaterThan(line 2060) andlessThan(line 2061), but the parent's validation whitelist (SQL.php:2046-2208) includesgreaterThanOrEqualandlessThanOrEqualas valid filter types.When these filter types are used, they will fall through to the
ELSE TRUEbranch (line 2064), causing incorrect filtering (keeping all values instead of applying the comparison).Apply this diff:
WHERE CASE :$conditionKey WHEN 'equal' THEN value = JSON_UNQUOTE(:$valueKey) WHEN 'notEqual' THEN value != JSON_UNQUOTE(:$valueKey) 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)) WHEN 'isNull' THEN value IS NULL WHEN 'isNotNull' THEN value IS NOT NULL ELSE TRUE ENDBased on past review comments.
🧹 Nitpick comments (4)
src/Database/Adapter/Postgres.php (1)
2592-2603: Optional: use target-prefixed ref in DATE ops (consistency).Use $columnRef on RHS when useTargetPrefix=true for UPSERT parity with other ops.
- return "{$quotedColumn} = {$quotedColumn} + (:$bindKey || ' days')::INTERVAL"; + return "{$quotedColumn} = {$columnRef} + (:$bindKey || ' days')::INTERVAL"; @@ - return "{$quotedColumn} = {$quotedColumn} - (:$bindKey || ' days')::INTERVAL"; + return "{$quotedColumn} = {$columnRef} - (:$bindKey || ' days')::INTERVAL";Also applies to: 2596-2600
src/Database/Adapter/SQL.php (1)
2236-2242: Use strict zero-checks in PHP fallback.Harden applyOperatorToValue against coercion.
- $divisor = $values[0] ?? 1; - return $divisor != 0 ? ($value ?? 0) / $divisor : ($value ?? 0); + $divisor = $values[0] ?? 1; + return $divisor !== 0 ? ($value ?? 0) / $divisor : ($value ?? 0); @@ - $divisor = $values[0] ?? 1; - return $divisor != 0 ? ($value ?? 0) % $divisor : ($value ?? 0); + $divisor = $values[0] ?? 1; + return $divisor !== 0 ? ($value ?? 0) % $divisor : ($value ?? 0);src/Database/Adapter/SQLite.php (1)
1285-1303: Consider distinguishing _uid violations from other unique constraints.Similar to the MariaDB adapter learning, only duplicate
_uidviolations should throwDuplicateException, while other unique constraint violations (including PRIMARY key and user-defined unique constraints) should throw aUniqueException.This would require:
- Importing
UniqueException- Parsing the error message to check if it references the
_uidconstraint- Throwing
DuplicateExceptiononly for_uidcollisions,UniqueExceptionfor othersThis distinction helps differentiate user-facing document duplicates from internal/user-defined unique constraint violations.
Based on learnings.
src/Database/Adapter/MariaDB.php (1)
1854-1857: Consider distinguishing _uid violations from other unique constraints.Per the design learning from PR #733, only duplicate
_uidviolations should throwDuplicateException, while other unique constraint violations (including PRIMARY key and user-defined unique constraints) should throwUniqueException.This would require:
- Importing
UniqueException- Parsing
$e->errorInfo[2]to check if the error message contains"for key '_uid'"- Throwing
DuplicateExceptiononly for_uidcollisions,UniqueExceptionfor othersThis distinction helps differentiate user-facing document duplicates (_uid collisions) from internal/user-defined unique constraint violations.
Based on learnings.
📜 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/SQL.php(9 hunks)src/Database/Adapter/SQLite.php(9 hunks)src/Database/Validator/Operator.php(1 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/MariaDB.phpsrc/Database/Adapter/SQLite.php
🧬 Code graph analysis (5)
src/Database/Adapter/SQL.php (2)
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 (2)
Adapter(16-1408)getSupportForOperators(1085-1085)
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 (4)
getOperatorSQL(2405-2608)bindOperatorParams(2619-2654)getPDOType(1897-1906)quote(2084-2087)src/Database/Adapter/SQL.php (4)
getOperatorSQL(1868-2037)bindOperatorParams(2047-2209)convertArrayToWKT(3060-3099)getPDOType(2323-2323)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
src/Database/Adapter/Postgres.php (3)
src/Database/Database.php (1)
Database(38-8559)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)
getSpatialAttributes(432-445)getOperatorSQL(1868-2037)getSpatialGeomFromText(1664-1676)bindOperatorParams(2047-2209)convertArrayToWKT(3060-3099)getPDOType(2323-2323)
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/SQLite.php (4)
src/Database/Adapter/MariaDB.php (4)
processException(1832-1898)getOperatorSQL(1914-2072)getPDOType(1712-1720)quote(1900-1903)src/Database/Adapter/SQL.php (9)
processException(2549-2552)getSpatialAttributes(432-445)getOperatorSQL(1868-2037)getSpatialGeomFromText(1664-1676)bindOperatorParams(2047-2209)convertArrayToWKT(3060-3099)getPDOType(2323-2323)getPDO(2311-2314)count(3335-3397)src/Database/Adapter.php (2)
count(825-825)quote(467-467)src/Database/Operator.php (1)
isOperator(664-667)
🪛 GitHub Actions: Linter
src/Database/Validator/Operator.php
[error] 1-1: Pint lint failed: PSR-12 no_whitespace_in_blank_line detected in Operator.php.
⏰ 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/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (14)
src/Database/Adapter/SQL.php (2)
1557-1565: Public capability surface: getSupportForOperators().Nice addition; clear feature detection.
1698-1706: All adapters correctly implement the updated signature.Verification confirms that all three concrete adapters (SQLite, Postgres, MariaDB) have been properly updated with the new
$operatorsparameter, matching the abstract method definition in SQL.php. No signature mismatches or compatibility issues exist.src/Database/Validator/Operator.php (2)
1-1: The review comment location is incorrect.The original comment flags lines 1-1, but the file output shows line 1 (
<?php) contains no trailing whitespace. The actual offending line with whitespace-only content is at line 272, which contains trailing spaces. The review comment is mislabeling the issue location.Likely an incorrect or invalid review comment.
273-281: Looking back at the script output you provided initially, I can complete the analysis without needing additional queries. Let me examine what's already there:From the output at src/Database/Adapter/SQLite.php:1418:
$comparisonTypes = ['equal', 'notEqual', 'greaterThan', 'greaterThanEqual', 'lessThan', 'lessThanEqual'];And src/Database/Adapter/SQLite.php:1692-1712 shows the switch statement that handles these exact names—there is no normalization or mapping of aliases before this point.
Similarly, src/Database/Adapter/SQL.php:2188-2192 shows the same list of conditions with identical names, with no alias handling.
The review comment's premise—"Adapters handle additional aliases"—is not supported by the code. The adapters contain only the primary condition names in their case statements, with no preceding logic to normalize incoming aliases like
'equals'→'equal'.Do not add aliases to the validator without first implementing alias normalization in the adapters.
Adding the suggested aliases (
equals,notEquals,greaterThanOrEqual,lessThanOrEqual,null,notNull) to the validator would allow invalid conditions to pass validation. When these conditions reach the adapters, they would either fail thein_array()check or bypass the switch case, resulting in runtime errors or silent failures—the exact "valid in validator, no-op in SQL" mismatch the review comment warns against.If aliases are intended to be supported, implement alias-to-primary-name normalization in the adapters first, then update the validator.
Likely an incorrect or invalid review comment.
src/Database/Adapter/SQLite.php (5)
11-18: LGTM!The new imports (LimitException, OperatorException, Operator) are appropriate for the operator support being added and are used throughout the file.
623-625: LGTM!Exception handling now correctly delegates to the centralized
processException()method, maintaining consistent error mapping across the adapter.
1368-1387: LGTM!The
getSupportForMathFunctions()check is a good defensive measure since SQLite's math functions (like POWER) require compile-time flag-DSQLITE_ENABLE_MATH_FUNCTIONS. The static caching avoids repeated checks.
1665-1733: LGTM!The
ARRAY_FILTERimplementation correctly handles all filter types:
isNull(line 1675) filters for null valuesisNotNull(line 1683) filters out null values- Numeric comparisons (lines 1717-1722) properly cast both sides to REAL for numeric ordering
- Text comparisons (lines 1723-1727) use direct value comparison
The filter type names match the parent's validation whitelist (
'isNull','isNotNull'), and numeric casting prevents text-based comparison issues (e.g., "10" < "2").
1769-1849: LGTM!The upsert implementation correctly:
- Uses separate
$opIndexfor operator placeholders (line 1794)- Checks
$operatorSQL !== nullbefore adding to update columns (line 1813)- Binds operator parameters in the same order as SQL generation (lines 1842-1846)
- Uses SQLite's
ON CONFLICTsyntax instead of MariaDB'sON DUPLICATE KEY UPDATEThe operator handling is consistent with
updateDocumentand avoids placeholder/binding misalignment.src/Database/Adapter/MariaDB.php (5)
11-17: LGTM!The new imports (LimitException, Operator) are appropriate for the operator support being added and are used throughout the file.
1115-1184: LGTM!The updateDocument changes correctly implement operator support:
- Uses separate
keyIndex(line 1115) andopIndex(line 1116) counters to avoid placeholder conflicts- Converts spatial arrays to WKT before binding (lines 1171-1174), addressing past review feedback
- Binds operator parameters with independent
opIndexForBindingcounter (line 1164)- Maintains proper separation between operators and regular attributes
1213-1292: LGTM!The upsert implementation correctly:
- Uses separate
$opIndexfor operator placeholders (line 1239)- Checks
$operatorSQL !== nullbefore adding to update columns (line 1258)- Binds operator parameters in the same attribute order as SQL generation (lines 1285-1289), addressing past review feedback
- Binds regular attribute values before operator parameters (lines 1278-1280)
This ensures deterministic ordering and keeps bindIndex alignment synchronized.
1865-1873: LGTM!The LimitException mappings correctly handle numeric out-of-range errors:
- Error code 22003 with errorInfo 1264 or 1690 (line 1866)
- Error code HY000 with errorInfo 1690 (line 1871)
These mappings provide better error classification for operator overflow/underflow scenarios.
1922-2047: LGTM!The operator SQL generation correctly implements:
- Numeric operators (INCREMENT, DECREMENT, MULTIPLY, DIVIDE, MODULO, POWER) with overflow prevention using CASE statements and COALESCE for NULL handling
- TOGGLE operator with NULL-safe boolean inversion (line 2000)
- CONCAT operator with NULL-safe string concatenation (line 2005)
- Array operators (ARRAY_INSERT, ARRAY_INTERSECT, ARRAY_DIFF, ARRAY_UNIQUE, ARRAY_REMOVE) using MariaDB's JSON_TABLE and JSON_ARRAYAGG functions
The implementations are MariaDB-specific and handle edge cases appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/Database/Adapter/Postgres.php (3)
2565-2579: ARRAY_FILTER: add alias support and fix >=/<= names + safer numeric cast.Support validator synonyms ('equals','notEquals','greaterThanOrEqual','lessThanOrEqual','null','notNull') and use TRIM with explicit cast to avoid subtle placeholder-cast issues.
- WHERE CASE :$conditionKey - WHEN 'equal' THEN value = :$valueKey::jsonb - WHEN 'notEqual' THEN value != :$valueKey::jsonb - WHEN 'greaterThan' THEN (value::text)::numeric > trim(both '\"' from :$valueKey::text)::numeric - WHEN 'greaterThanEqual' THEN (value::text)::numeric >= trim(both '\"' from :$valueKey::text)::numeric - WHEN 'lessThan' THEN (value::text)::numeric < trim(both '\"' from :$valueKey::text)::numeric - WHEN 'lessThanEqual' THEN (value::text)::numeric <= trim(both '\"' from :$valueKey::text)::numeric - WHEN 'isNull' THEN value = 'null'::jsonb - WHEN 'isNotNull' THEN value != 'null'::jsonb + WHERE CASE :$conditionKey + WHEN 'equal' THEN value = :$valueKey::jsonb + WHEN 'equals' THEN value = :$valueKey::jsonb + WHEN 'notEqual' THEN value != :$valueKey::jsonb + WHEN 'notEquals' THEN value != :$valueKey::jsonb + WHEN 'greaterThan' THEN (value::text)::numeric > TRIM(BOTH '\"' FROM (:$valueKey)::text)::numeric + WHEN 'greaterThanOrEqual' THEN (value::text)::numeric >= TRIM(BOTH '\"' FROM (:$valueKey)::text)::numeric + WHEN 'lessThan' THEN (value::text)::numeric < TRIM(BOTH '\"' FROM (:$valueKey)::text)::numeric + WHEN 'lessThanOrEqual' THEN (value::text)::numeric <= TRIM(BOTH '\"' FROM (:$valueKey)::text)::numeric + WHEN 'isNull' THEN value = 'null'::jsonb + WHEN 'null' THEN value = 'null'::jsonb + WHEN 'isNotNull' THEN value != 'null'::jsonb + WHEN 'notNull' THEN value != 'null'::jsonb ELSE TRUE END
2456-2466: DIVIDE underflow guard is not sign-aware (negative divisors).For negative divisors the inequality flips; current check can cap incorrectly. Make the guard sign-aware.
- return "{$quotedColumn} = CASE - WHEN CAST(:$bindKey AS NUMERIC) != 0 AND COALESCE({$columnRef}, 0) / CAST(:$bindKey AS NUMERIC) <= CAST(:$minKey AS NUMERIC) THEN CAST(:$minKey AS NUMERIC) - ELSE COALESCE({$columnRef}, 0) / CAST(:$bindKey AS NUMERIC) - END"; + return "{$quotedColumn} = CASE + WHEN CAST(:$bindKey AS NUMERIC) = 0 THEN COALESCE({$columnRef}, 0) -- divide-by-zero handled by DB error otherwise + WHEN CAST(:$bindKey AS NUMERIC) > 0 AND COALESCE({$columnRef}, 0) / CAST(:$bindKey AS NUMERIC) <= CAST(:$minKey AS NUMERIC) THEN CAST(:$minKey AS NUMERIC) + WHEN CAST(:$bindKey AS NUMERIC) < 0 AND COALESCE({$columnRef}, 0) / CAST(:$bindKey AS NUMERIC) >= CAST(:$minKey AS NUMERIC) THEN CAST(:$minKey AS NUMERIC) + ELSE COALESCE({$columnRef}, 0) / CAST(:$bindKey AS NUMERIC) + END";
2624-2653: Add array-size caps for array ops (DoS guard).Large arrays can balloon CPU/allocations. Enforce MAX_ARRAY_OPERATOR_SIZE before binding.
case Operator::TYPE_ARRAY_APPEND: case Operator::TYPE_ARRAY_PREPEND: + if (\count($values) > self::MAX_ARRAY_OPERATOR_SIZE) { + throw new DatabaseException("Array size " . \count($values) . " exceeds maximum allowed size of " . self::MAX_ARRAY_OPERATOR_SIZE . " for array operations"); + } $arrayValue = json_encode($values); @@ case Operator::TYPE_ARRAY_INTERSECT: case Operator::TYPE_ARRAY_DIFF: + if (\count($values) > self::MAX_ARRAY_OPERATOR_SIZE) { + throw new DatabaseException("Array size " . \count($values) . " exceeds maximum allowed size of " . self::MAX_ARRAY_OPERATOR_SIZE . " for array operations"); + } $arrayValue = json_encode($values);src/Database/Adapter/MariaDB.php (2)
1854-1857: Only _uid collisions ⇒ DuplicateException; others ⇒ UniqueException.Split mapping so user doc duplicates raise DuplicateException; other unique/PRIMARY violations raise UniqueException.
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] ?? $e->getMessage(); + if (\stripos($msg, '_uid') !== false) { + return new DuplicateException('Document already exists', $e->getCode(), $e); + } + return new UniqueException('Unique constraint violated', $e->getCode(), $e); + }
2049-2068: ARRAY_FILTER: add >=/<= aliases and keep JSON_UNQUOTE numeric casting.Support 'greaterThanOrEqual'/'lessThanOrEqual' alongside existing names for parity with validators.
WHERE CASE :$conditionKey WHEN 'equal' THEN value = JSON_UNQUOTE(:$valueKey) WHEN 'notEqual' THEN value != JSON_UNQUOTE(:$valueKey) WHEN 'greaterThan' THEN CAST(value AS DECIMAL(65,30)) > CAST(JSON_UNQUOTE(:$valueKey) AS DECIMAL(65,30)) - WHEN 'greaterThanEqual' THEN CAST(value AS DECIMAL(65,30)) >= CAST(JSON_UNQUOTE(:$valueKey) AS DECIMAL(65,30)) + WHEN 'greaterThanEqual' 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 'lessThanEqual' THEN CAST(value AS DECIMAL(65,30)) <= CAST(JSON_UNQUOTE(:$valueKey) AS DECIMAL(65,30)) + WHEN 'lessThanEqual' 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)) WHEN 'isNull' THEN value IS NULL WHEN 'isNotNull' THEN value IS NOT NULL
🧹 Nitpick comments (4)
src/Database/Adapter/SQLite.php (3)
10-15: Differentiate Duplicate vs Unique exceptions (_uid-only => DuplicateException).Map UNIQUE violations on _uid to DuplicateException; all other unique/PRIMARY violations to UniqueException. Keeps semantics aligned with other adapters.
use Utopia\Database\Exception\Duplicate as DuplicateException; +use Utopia\Database\Exception\Unique as UniqueException; @@ - if ( + 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); + // Heuristic: treat only _uid collisions as DuplicateException + if (stripos($message, '._uid') !== false || stripos($message, '_uid') !== false) { + return new DuplicateException('Document already exists', $e->getCode(), $e); + } + return new UniqueException('Unique constraint violated', $e->getCode(), $e); } }Also applies to: 1298-1316
1015-1026: Silence PHPMD unused-parameter warning for $srid.Override must keep signature; add suppression to satisfy static analysis.
/** * Override getSpatialGeomFromText to return placeholder unchanged for SQLite * SQLite does not support ST_GeomFromText, so we return the raw placeholder * * @param string $wktPlaceholder * @param int|null $srid * @return string + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ protected function getSpatialGeomFromText(string $wktPlaceholder, ?int $srid = null): string
1704-1743: ARRAY_FILTER: add aliases and numeric-cast parity with other adapters.Support ('equals','notEquals','greaterThanOrEqual','lessThanOrEqual','null','notNull') for consistency.
- case 'equal': - case 'notEqual': - case 'greaterThan': - case 'greaterThanEqual': - case 'lessThan': - case 'lessThanEqual': + case 'equal': + case 'equals': + case 'notEqual': + case 'notEquals': + case 'greaterThan': + case 'greaterThanEqual': + case 'greaterThanOrEqual': + case 'lessThan': + case 'lessThanEqual': + case 'lessThanOrEqual': @@ - $operator = match ($filterType) { + $operator = match ($filterType) { 'equal' => '=', + 'equals' => '=', 'notEqual' => '!=', + 'notEquals' => '!=', 'greaterThan' => '>', - 'greaterThanEqual' => '>=', + 'greaterThanEqual', 'greaterThanOrEqual' => '>=', 'lessThan' => '<', - 'lessThanEqual' => '<=', + 'lessThanEqual', 'lessThanOrEqual' => '<=', default => throw new OperatorException('Unsupported filter type: ' . $filterType), };src/Database/Adapter/MariaDB.php (1)
1865-1873: Deduplicate numeric range mapping (1690).Two branches handle the same condition; keep one.
- // Numeric value out of range - if ($e->getCode() === '22003' && isset($e->errorInfo[1]) && ($e->errorInfo[1] === 1264 || $e->errorInfo[1] === 1690)) { - return new LimitException('Value out of range', $e->getCode(), $e); - } - - // Numeric value out of range - if ($e->getCode() === 'HY000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1690) { - return new LimitException('Value is out of range', $e->getCode(), $e); - } + // Numeric value out of range + if ( + ( $e->getCode() === '22003' && isset($e->errorInfo[1]) && ($e->errorInfo[1] === 1264 || $e->errorInfo[1] === 1690) ) + || ( $e->getCode() === 'HY000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1690 ) + ) { + return new LimitException('Value out of range', $e->getCode(), $e); + }
📜 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/Postgres.php(10 hunks)src/Database/Adapter/SQLite.php(10 hunks)src/Database/Validator/Operator.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Validator/Operator.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 (3)
src/Database/Adapter/Postgres.php (5)
src/Database/Operator.php (4)
Operator(14-698)increment(409-416)getMethod(139-142)getValues(155-158)src/Database/Adapter/SQL.php (6)
getSpatialAttributes(432-445)getOperatorSQL(1868-2037)getSpatialGeomFromText(1664-1676)bindOperatorParams(2047-2209)convertArrayToWKT(3060-3099)getPDOType(2323-2323)src/Database/Adapter/MariaDB.php (3)
getOperatorSQL(1914-2074)getPDOType(1712-1720)quote(1900-1903)src/Database/Adapter/SQLite.php (3)
getOperatorSQL(1465-1768)getSpatialGeomFromText(1023-1026)bindOperatorParams(1411-1444)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
src/Database/Adapter/MariaDB.php (3)
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-2608)bindOperatorParams(2619-2654)getPDOType(1895-1904)quote(2082-2085)src/Database/Adapter/SQL.php (4)
getOperatorSQL(1868-2037)bindOperatorParams(2047-2209)convertArrayToWKT(3060-3099)getPDOType(2323-2323)
src/Database/Adapter/SQLite.php (3)
src/Database/Adapter/SQL.php (7)
getSpatialAttributes(432-445)getOperatorSQL(1868-2037)getSpatialGeomFromText(1664-1676)bindOperatorParams(2047-2209)getPDOType(2323-2323)getPDO(2311-2314)getUpsertStatement(1698-1706)src/Database/Adapter.php (2)
updateDocument(725-725)quote(467-467)src/Database/Operator.php (1)
isOperator(664-667)
🪛 PHPMD (2.15.0)
src/Database/Adapter/SQLite.php
1023-1023: Avoid unused parameters such as '$srid'. (undefined)
(UnusedFormalParameter)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Database/Adapter/SQL.php (2)
1989-1994: Fix ARRAY_INSERT to insert typed JSON value.The bound value is JSON-encoded, but inserting with a plain placeholder treats it as a string. Use
JSON_EXTRACTto insert the value as typed JSON.Apply this diff:
- return "{$quotedColumn} = JSON_INSERT({$quotedColumn}, CONCAT('$[', :$indexKey, ']'), :$valueKey)"; + return "{$quotedColumn} = JSON_INSERT({$quotedColumn}, CONCAT('$[', :$indexKey, ']'), JSON_EXTRACT(:$valueKey, '$'))";
2020-2037: Add missing conditions and JSON_EXTRACT to ARRAY_FILTER CASE.The CASE statement is missing
greaterThanOrEqualandlessThanOrEqualconditions that are whitelisted inbindOperatorParams(lines 2192-2195). Also, comparisons should useJSON_EXTRACTto properly compare JSON values.Apply this diff:
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 + WHEN 'equal' THEN value = JSON_EXTRACT(:$valueKey, '$') + WHEN 'notEqual' 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 'isNotNull' THEN value IS NOT NULL ELSE TRUE END
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Adapter/SQL.php(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/SQL.php (4)
src/Database/Database.php (1)
Database(38-8559)src/Database/Validator/Operator.php (1)
Operator(10-383)src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)extractOperators(675-696)src/Database/Adapter.php (2)
Adapter(16-1408)getSupportForOperators(1085-1085)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Postgres)
🔇 Additional comments (8)
src/Database/Adapter/SQL.php (8)
16-16: LGTM!The Operator import and MAX_ARRAY_OPERATOR_SIZE constant are well-defined and documented. The limit of 10,000 elements provides reasonable protection against memory exhaustion in JSON_TABLE operations.
Also applies to: 24-28
490-574: LGTM!The updateDocuments method correctly implements operator support with separate bind index counters for regular attributes and operators, preventing the bind-index mismatch issue. The operator extraction, SQL generation, and parameter binding logic are properly structured.
1561-1569: LGTM!The getSupportForOperators method correctly follows the adapter pattern for capability flags and is consistent with other support methods in the codebase.
1699-1710: LGTM!The getUpsertStatement signature correctly adds operator support while maintaining backward compatibility with a default empty array parameter.
2043-2213: LGTM!The bindOperatorParams method correctly implements parameter binding for all operator types with proper safeguards:
- Array size validation prevents memory exhaustion
- Condition whitelist prevents SQL injection in CASE statements
- Appropriate type handling for each operator
2215-2309: LGTM!The applyOperatorToValue method correctly implements PHP-side operator logic for computing initial values when upserting new documents. Division-by-zero protection and NULL handling are properly implemented.
2720-3054: LGTM! Excellent optimization strategy.The rewritten upsertDocuments method implements a smart two-path approach:
- Fast path (lines 2761-2838): Zero overhead for non-operator upserts, maintaining original bulk insert performance
- Operator path (lines 2839-2970): Groups documents by operator signature to batch similar operations
The logic for applying operators to attribute defaults for new documents (lines 2891-2896) correctly ensures operators work even when only operators are provided without regular attributes.
3064-3103: LGTM!The convertArrayToWKT helper correctly validates and converts array representations of POINT, LINESTRING, and POLYGON geometries to WKT format with appropriate error handling.
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 (2)
218-237: Don’t expect exceptions on schemaless adapters; guard tests with getSupportForAttributes().These three blocks will fail when the adapter reports no attribute support (schemaless). Wrap the exception expectation so schemaless paths do not call
$this->fail(...). Minimal patch below for each block.@@ - try { - $database->createDocument('documents', new Document([ + if ($database->getAdapter()->getSupportForAttributes()) { + try { + $database->createDocument('documents', new Document([ 'string' => '', 'integer_signed' => 0, 'integer_unsigned' => 0, 'bigint_signed' => 0, 'bigint_unsigned' => 0, 'float_signed' => 0, 'float_unsigned' => -5.55, 'boolean' => true, 'colors' => [], 'empty' => [], - ])); - $this->fail('Failed to throw exception'); - } catch (Throwable $e) { - if ($database->getAdapter()->getSupportForAttributes()) { - $this->assertTrue($e instanceof StructureException); - $this->assertStringContainsString('Invalid document structure: Attribute "float_unsigned" has invalid type. Value must be a valid range between 0 and', $e->getMessage()); - } - } + ])); + $this->fail('Failed to throw exception'); + } catch (Throwable $e) { + $this->assertInstanceOf(StructureException::class, $e); + $this->assertStringContainsString('Invalid document structure: Attribute "float_unsigned" has invalid type. Value must be a valid range between 0 and', $e->getMessage()); + } + } else { + // Schemaless: no validation error expected + $database->createDocument('documents', new Document([ + 'string' => '', + 'integer_signed' => 0, + 'integer_unsigned' => 0, + 'bigint_signed' => 0, + 'bigint_unsigned' => 0, + 'float_signed' => 0, + 'float_unsigned' => -5.55, + 'boolean' => true, + 'colors' => [], + 'empty' => [], + ])); + $this->assertTrue(true); + } @@ - try { - $database->createDocument('documents', new Document([ + if ($database->getAdapter()->getSupportForAttributes()) { + try { + $database->createDocument('documents', new Document([ 'string' => '', 'integer_signed' => 0, 'integer_unsigned' => 0, 'bigint_signed' => 0, 'bigint_unsigned' => -10, 'float_signed' => 0, 'float_unsigned' => 0, 'boolean' => true, 'colors' => [], 'empty' => [], - ])); - $this->fail('Failed to throw exception'); - } catch (Throwable $e) { - if ($database->getAdapter()->getSupportForAttributes()) { - $this->assertTrue($e instanceof StructureException); - $this->assertEquals('Invalid document structure: Attribute "bigint_unsigned" has invalid type. Value must be a valid range between 0 and 9,223,372,036,854,775,807', $e->getMessage()); - } - } + ])); + $this->fail('Failed to throw exception'); + } catch (Throwable $e) { + $this->assertInstanceOf(StructureException::class, $e); + $this->assertEquals('Invalid document structure: Attribute "bigint_unsigned" has invalid type. Value must be a valid range between 0 and 9,223,372,036,854,775,807', $e->getMessage()); + } + } else { + // Schemaless: no validation error expected + $database->createDocument('documents', new Document([ + 'string' => '', + 'integer_signed' => 0, + 'integer_unsigned' => 0, + 'bigint_signed' => 0, + 'bigint_unsigned' => -10, + 'float_signed' => 0, + 'float_unsigned' => 0, + 'boolean' => true, + 'colors' => [], + 'empty' => [], + ])); + $this->assertTrue(true); + } @@ - try { - $database->createDocument('documents', new Document([ + if ($database->getAdapter()->getSupportForAttributes()) { + try { + $database->createDocument('documents', new Document([ '$sequence' => '0', '$permissions' => [], 'string' => '', 'integer_signed' => 1, 'integer_unsigned' => 1, 'bigint_signed' => 1, 'bigint_unsigned' => 1, 'float_signed' => 1, 'float_unsigned' => 1, 'boolean' => true, 'colors' => [], 'empty' => [], 'with-dash' => '', - ])); - $this->fail('Failed to throw exception'); - } catch (Throwable $e) { - if ($database->getAdapter()->getSupportForAttributes()) { - $this->assertTrue($e instanceof StructureException); - $this->assertEquals('Invalid document structure: Attribute "$sequence" has invalid type. Invalid sequence value', $e->getMessage()); - } - } + ])); + $this->fail('Failed to throw exception'); + } catch (Throwable $e) { + $this->assertInstanceOf(StructureException::class, $e); + $this->assertEquals('Invalid document structure: Attribute "$sequence" has invalid type. Invalid sequence value', $e->getMessage()); + } + } else { + // Schemaless: no validation error expected for internal attributes + $database->createDocument('documents', new Document([ + '$sequence' => '0', + '$permissions' => [], + 'string' => '', + 'integer_signed' => 1, + 'integer_unsigned' => 1, + 'bigint_signed' => 1, + 'bigint_unsigned' => 1, + 'float_signed' => 1, + 'float_unsigned' => 1, + 'boolean' => true, + 'colors' => [], + 'empty' => [], + 'with-dash' => '', + ])); + $this->assertTrue(true); + }Based on learnings.
Also applies to: 239-258, 260-282
1920-1927: Standardize fulltext "wildcard" support method naming.The canonical method name defined in the adapter interface is
getSupportForFulltextWildcardIndex()(lowercase 'c'). The test file inconsistently uses both variants:
- Line 1923: uses uppercase variant (incorrect)
- Line 1972: uses lowercase variant (correct)
- Line 3411: uses uppercase variant (incorrect)
Replace the uppercase
WildCardvariant with lowercaseWildcardat lines 1923 and 3411 to match the canonical interface definition.src/Database/Adapter/MariaDB.php (1)
880-891: createDocument binding loop drops attribute name; spatial arrays not converted.The loop iterates values only, but uses $attribute (undefined) and never converts spatial arrays to WKT. Bind per key and handle spatial arrays like updateDocument.
- $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 $attr => $value) { + if (\in_array($attr, $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/Postgres.php (2)
2570-2585: arrayFilter: broaden condition aliases and use safe TRIM/CAST form.Support validator synonyms (equals/notEquals/gte/lte/null/notNull, short aliases) and prefer CAST(TRIM(...)) to avoid cast parsing edge cases.
- WHERE CASE :$conditionKey - WHEN 'equal' THEN value = :$valueKey::jsonb - WHEN 'notEqual' THEN value != :$valueKey::jsonb - WHEN 'greaterThan' THEN (value::text)::numeric > trim(both '\"' from :$valueKey::text)::numeric - WHEN 'greaterThanEqual' THEN (value::text)::numeric >= trim(both '\"' from :$valueKey::text)::numeric - WHEN 'lessThan' THEN (value::text)::numeric < trim(both '\"' from :$valueKey::text)::numeric - WHEN 'lessThanEqual' THEN (value::text)::numeric <= trim(both '\"' from :$valueKey::text)::numeric - WHEN 'isNull' THEN value = 'null'::jsonb - WHEN 'isNotNull' THEN value != 'null'::jsonb + WHERE CASE :$conditionKey + WHEN 'equal' THEN value = :$valueKey::jsonb + WHEN 'equals' THEN value = :$valueKey::jsonb + WHEN 'eq' THEN value = :$valueKey::jsonb + WHEN 'notEqual' THEN value != :$valueKey::jsonb + WHEN 'notEquals' THEN value != :$valueKey::jsonb + WHEN 'ne' THEN value != :$valueKey::jsonb + WHEN 'greaterThan' THEN (value::text)::numeric > CAST(TRIM(BOTH '\"' FROM (:$valueKey)::text) AS numeric) + WHEN 'greaterThanOrEqual' THEN (value::text)::numeric >= CAST(TRIM(BOTH '\"' FROM (:$valueKey)::text) AS numeric) + WHEN 'gte' 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) + WHEN 'lessThanOrEqual' THEN (value::text)::numeric <= CAST(TRIM(BOTH '\"' FROM (:$valueKey)::text) AS numeric) + WHEN 'lte' THEN (value::text)::numeric <= CAST(TRIM(BOTH '\"' FROM (:$valueKey)::text) AS numeric) + WHEN 'isNull' THEN value = 'null'::jsonb + WHEN 'null' THEN value = 'null'::jsonb + WHEN 'isNotNull' THEN value != 'null'::jsonb + WHEN 'notNull' THEN value != 'null'::jsonb ELSE TRUE END
2624-2659: Restore array-size caps in Postgres binding override (DoS guard).The override skips MAX_ARRAY_OPERATOR_SIZE checks present in the parent, allowing oversized arrays. Reinstate caps before encoding/binding.
Apply:
@@ - case Operator::TYPE_ARRAY_APPEND: - case Operator::TYPE_ARRAY_PREPEND: + case Operator::TYPE_ARRAY_APPEND: + case Operator::TYPE_ARRAY_PREPEND: + if (\count($values) > self::MAX_ARRAY_OPERATOR_SIZE) { + throw new DatabaseException("Array size " . \count($values) . " exceeds maximum allowed size of " . self::MAX_ARRAY_OPERATOR_SIZE . " for array operations"); + } $arrayValue = json_encode($values); $bindKey = "op_{$bindIndex}"; $stmt->bindValue(':' . $bindKey, $arrayValue, \PDO::PARAM_STR); $bindIndex++; break; @@ - case Operator::TYPE_ARRAY_INTERSECT: - case Operator::TYPE_ARRAY_DIFF: + case Operator::TYPE_ARRAY_INTERSECT: + case Operator::TYPE_ARRAY_DIFF: + if (\count($values) > self::MAX_ARRAY_OPERATOR_SIZE) { + throw new DatabaseException("Array size " . \count($values) . " exceeds maximum allowed size of " . self::MAX_ARRAY_OPERATOR_SIZE . " for array operations"); + } $arrayValue = json_encode($values); $bindKey = "op_{$bindIndex}"; $stmt->bindValue(':' . $bindKey, $arrayValue, \PDO::PARAM_STR); $bindIndex++; break;src/Database/Adapter/SQL.php (3)
1981-1985: ARRAY_REMOVE can set column to NULL when value not found.Wrap JSON_REMOVE with IFNULL to preserve the original array. This was flagged earlier and remains.
- 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} + )";
1989-1995: ARRAY_INSERT binds JSON as a quoted string. Insert typed JSON.Wrap the value placeholder with JSON_EXTRACT so the inserted element is JSON-typed, not a string. Previously raised; still present here.
- return "{$quotedColumn} = JSON_INSERT({$quotedColumn}, CONCAT('$[', :$indexKey, ']'), :$valueKey)"; + return "{$quotedColumn} = JSON_INSERT( + {$quotedColumn}, + CONCAT('$[', :$indexKey, ']'), + JSON_EXTRACT(:$valueKey, '$') + )";
2020-2037: ARRAY_FILTER: condition mismatch and JSON comparison bugs.CASE doesn’t recognise several allowed strings and compares against raw placeholders instead of JSON values. Align CASE with the whitelist and 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 'greaterThanEqual' 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 'lessThanEqual' 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- $validConditions = [ - 'equal', 'notEqual', // Comparison - 'greaterThan', 'greaterThanEqual', 'lessThan', 'lessThanEqual', // Numeric - 'isNull', 'isNotNull' // Null checks - ]; + $validConditions = [ + // equality + 'equal','equals','notEqual','notEquals', + // numeric + 'greaterThan','greaterThanEqual','greaterThanOrEqual', + 'lessThan','lessThanEqual','lessThanOrEqual', + // null checks + 'isNull','null','isNotNull','notNull' + ];Also applies to: 2187-2211
src/Database/Adapter/MariaDB.php (1)
11-11: Map duplicate key errors: only _uid ⇒ DuplicateException; others ⇒ UniqueException.Per design, unique/PRIMARY violations not on _uid should throw UniqueException. Import and map accordingly. Based on learnings
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); + }Also applies to: 1859-1863
🧹 Nitpick comments (4)
tests/e2e/Adapter/Scopes/DocumentTests.php (2)
311-314: Use correctQuery::...casing for consistency and tooling.Two occurrences use
query::...(lowercase). While PHP is case-insensitive, consistent casing avoids static analysis noise.- $documentIdNull = $database->findOne('documents', [ - query::isNull('id') - ]); + $documentIdNull = $database->findOne('documents', [ + Query::isNull('id') + ]); @@ - $documentId0 = $database->findOne('documents', [ - query::equal('id', [$sequence]) - ]); + $documentId0 = $database->findOne('documents', [ + Query::equal('id', [$sequence]) + ]);Also applies to: 349-352
9-9: Unused import: Utopia\Database\DateTime.Appears unused in this test file. If not needed, remove to keep imports tidy.
-use Utopia\Database\DateTime;src/Database/Adapter/SQL.php (2)
1986-2019: Prefer JSON_ARRAY() over string '[]' for empty results.Use typed JSON to avoid implicit casts.
Examples:
- ), '[]')"; + ), JSON_ARRAY())";Apply similarly to ARRAY_UNIQUE/INTERSECT/DIFF empty cases.
Also applies to: 1999-2006, 2011-2018
2215-2310: applyOperatorToValue: ARRAY_FILTER not applied.Returning the original array may surprise upserts creating new docs purely via operators. Either implement the filter semantics or document that ARRAY_FILTER is SQL-only.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/MariaDB.php(10 hunks)src/Database/Adapter/Mongo.php(1 hunks)src/Database/Adapter/Postgres.php(11 hunks)src/Database/Adapter/SQL.php(9 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(1 hunks)
🧰 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.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 (6)
src/Database/Adapter/Mongo.php (5)
src/Database/Adapter.php (2)
getSupportForOperators(1085-1085)getSupportForIntegerBooleans(1413-1413)src/Database/Adapter/SQL.php (1)
getSupportForOperators(1566-1569)src/Database/Adapter/Pool.php (1)
getSupportForOperators(403-406)src/Database/Adapter/MariaDB.php (1)
getSupportForIntegerBooleans(1773-1776)src/Database/Adapter/Postgres.php (1)
getSupportForIntegerBooleans(2000-2003)
tests/e2e/Adapter/Scopes/DocumentTests.php (1)
src/Database/Database.php (1)
Database(38-8559)
src/Database/Adapter.php (5)
src/Database/Adapter/Mongo.php (2)
getSupportForOperators(2897-2900)getSupportForIntegerBooleans(2907-2910)src/Database/Adapter/SQL.php (1)
getSupportForOperators(1566-1569)src/Database/Adapter/Pool.php (1)
getSupportForOperators(403-406)src/Database/Adapter/MariaDB.php (1)
getSupportForIntegerBooleans(1773-1776)src/Database/Adapter/Postgres.php (1)
getSupportForIntegerBooleans(2000-2003)
src/Database/Adapter/Postgres.php (4)
src/Database/Validator/Operator.php (1)
Operator(10-383)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)
getSpatialAttributes(436-449)getOperatorSQL(1872-2041)getSpatialGeomFromText(1668-1680)bindOperatorParams(2051-2213)convertArrayToWKT(3068-3107)getPDOType(2327-2327)src/Database/Adapter.php (3)
filter(1239-1248)getSupportForIntegerBooleans(1413-1413)quote(467-467)
src/Database/Adapter/MariaDB.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.php (3)
filter(1239-1248)getSupportForIntegerBooleans(1413-1413)quote(467-467)src/Database/Adapter/Postgres.php (5)
getOperatorSQL(2408-2613)bindOperatorParams(2624-2659)getPDOType(1895-1904)getSupportForIntegerBooleans(2000-2003)quote(2087-2090)src/Database/Adapter/SQL.php (4)
getOperatorSQL(1872-2041)bindOperatorParams(2051-2213)convertArrayToWKT(3068-3107)getPDOType(2327-2327)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
src/Database/Adapter/SQL.php (7)
src/Database/Database.php (1)
Database(38-8559)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-1415)filter(1239-1248)quote(467-467)getSupportForOperators(1085-1085)getSupportForIntegerBooleans(1413-1413)src/Database/Adapter/MariaDB.php (5)
getOperatorSQL(1919-2079)quote(1905-1908)getPDOType(1712-1720)getSupportForIntegerBooleans(1773-1776)getUpsertStatement(1213-1292)src/Database/Adapter/Postgres.php (7)
getOperatorSQL(2408-2613)quote(2087-2090)bindOperatorParams(2624-2659)Postgres(21-2660)getPDOType(1895-1904)getSupportForIntegerBooleans(2000-2003)getUpsertStatement(1336-1415)src/Database/Adapter/SQLite.php (3)
getOperatorSQL(1465-1768)bindOperatorParams(1411-1444)getUpsertStatement(1782-1862)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
⏰ 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 (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Unit Test
🔇 Additional comments (7)
src/Database/Adapter/Mongo.php (1)
2892-2911: Capabilities: sane defaults for Mongo.Returning false for operators and integer booleans matches Mongo behavior. LGTM.
Please confirm Pool also delegates getSupportForIntegerBooleans(), similar to getSupportForOperators(), so capability checks work uniformly.
src/Database/Adapter/Postgres.php (2)
2000-2003: Capability: integer booleans — correct for Postgres.Returning false is correct; Postgres has native BOOLEAN. LGTM.
2065-2074: Error mapping: map overflow/DateTime to LimitException — good addition.Improves caller semantics for operator overflows. LGTM.
src/Database/Adapter.php (1)
1080-1085: No action required — all adapters properly implement or inherit getSupportForOperators().Verification confirms the BC surface expansion is correctly handled. All 7 adapters implement or inherit the method through the class hierarchy: SQL and Mongo/Pool provide explicit implementations, while MariaDB, MySQL, Postgres, and SQLite inherit from SQL (either directly or transitively via MariaDB). No adapters are missing the method.
src/Database/Adapter/SQL.php (1)
490-526: LGTM: separate key_/op_ counters and aligned binding.Using distinct counters for placeholders and binding them in attribute order fixes prior desync issues. Trailing comma trim is also correct.
Also applies to: 549-574
src/Database/Adapter/MariaDB.php (2)
1115-1143: LGTM: operator separation and binding order in updateDocument.Separate key_/op_ indices and attribute-order binding resolve prior placeholder mismatches; spatial WKT conversion in binding is correct.
Also applies to: 1163-1183
1278-1290: LGTM: bind operators in attribute order.Binding operator params in the same attribute iteration keeps opIndex aligned with SQL generation.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/Database/Adapter/Postgres.php (2)
2521-2593: Wrap all jsonb_array_elements calls with COALESCE to prevent NULL errors.
jsonb_array_elements({$columnRef})will raise a PostgreSQL error if$columnRefis NULL. The outerCOALESCEaround the entireSELECTstatement doesn't prevent this—the error occurs before the SELECT can return NULL.This affects all array operators: ARRAY_UNIQUE (line 2524), ARRAY_REMOVE (line 2532), ARRAY_INSERT (lines 2545, 2551), ARRAY_INTERSECT (line 2561), ARRAY_DIFF (line 2570), and ARRAY_FILTER (line 2581).
Apply this diff to wrap each occurrence:
case Operator::TYPE_ARRAY_UNIQUE: return "{$quotedColumn} = COALESCE(( SELECT jsonb_agg(DISTINCT value) - FROM jsonb_array_elements({$columnRef}) AS value + FROM jsonb_array_elements(COALESCE({$columnRef}, '[]'::jsonb)) AS value ), '[]'::jsonb)"; case Operator::TYPE_ARRAY_REMOVE: $bindKey = "op_{$bindIndex}"; $bindIndex++; return "{$quotedColumn} = COALESCE(( SELECT jsonb_agg(value) - FROM jsonb_array_elements({$columnRef}) AS value + FROM jsonb_array_elements(COALESCE({$columnRef}, '[]'::jsonb)) AS value WHERE value != :$bindKey::jsonb ), '[]'::jsonb)"; case Operator::TYPE_ARRAY_INSERT: $indexKey = "op_{$bindIndex}"; $bindIndex++; $valueKey = "op_{$bindIndex}"; $bindIndex++; return "{$quotedColumn} = ( SELECT jsonb_agg(value ORDER BY idx) FROM ( SELECT value, idx - FROM jsonb_array_elements({$columnRef}) WITH ORDINALITY AS t(value, idx) + FROM jsonb_array_elements(COALESCE({$columnRef}, '[]'::jsonb)) WITH ORDINALITY AS t(value, idx) WHERE idx - 1 < :$indexKey UNION ALL SELECT :$valueKey::jsonb AS value, :$indexKey + 1 AS idx UNION ALL SELECT value, idx + 1 - FROM jsonb_array_elements({$columnRef}) WITH ORDINALITY AS t(value, idx) + FROM jsonb_array_elements(COALESCE({$columnRef}, '[]'::jsonb)) WITH ORDINALITY AS t(value, idx) WHERE idx - 1 >= :$indexKey ) AS combined )"; case Operator::TYPE_ARRAY_INTERSECT: $bindKey = "op_{$bindIndex}"; $bindIndex++; return "{$quotedColumn} = COALESCE(( SELECT jsonb_agg(value) - FROM jsonb_array_elements({$columnRef}) AS value + FROM jsonb_array_elements(COALESCE({$columnRef}, '[]'::jsonb)) AS value WHERE value IN (SELECT jsonb_array_elements(:$bindKey::jsonb)) ), '[]'::jsonb)"; case Operator::TYPE_ARRAY_DIFF: $bindKey = "op_{$bindIndex}"; $bindIndex++; return "{$quotedColumn} = COALESCE(( SELECT jsonb_agg(value) - FROM jsonb_array_elements({$columnRef}) AS value + FROM jsonb_array_elements(COALESCE({$columnRef}, '[]'::jsonb)) AS value WHERE value NOT IN (SELECT jsonb_array_elements(:$bindKey::jsonb)) ), '[]'::jsonb)"; case Operator::TYPE_ARRAY_FILTER: $conditionKey = "op_{$bindIndex}"; $bindIndex++; $valueKey = "op_{$bindIndex}"; $bindIndex++; return "{$quotedColumn} = COALESCE(( SELECT jsonb_agg(value) - FROM jsonb_array_elements({$columnRef}) AS value + FROM jsonb_array_elements(COALESCE({$columnRef}, '[]'::jsonb)) AS value WHERE CASE :$conditionKey
2623-2658: Add MAX_ARRAY_OPERATOR_SIZE checks to prevent DoS attacks.The overridden
bindOperatorParamsmethod doesn't validate array sizes before JSON encoding. Large arrays could consume excessive memory or cause DoS. The parent implementation inSQL.phphas these checks, but they're bypassed by this override.Apply this diff to add size validation:
protected function bindOperatorParams(\PDOStatement $stmt, Operator $operator, int &$bindIndex): void { $method = $operator->getMethod(); $values = $operator->getValues(); switch ($method) { case Operator::TYPE_ARRAY_APPEND: case Operator::TYPE_ARRAY_PREPEND: + if (\count($values) > self::MAX_ARRAY_OPERATOR_SIZE) { + throw new DatabaseException("Array size " . \count($values) . " exceeds maximum allowed size of " . self::MAX_ARRAY_OPERATOR_SIZE . " for array operations"); + } $arrayValue = json_encode($values); $bindKey = "op_{$bindIndex}"; $stmt->bindValue(':' . $bindKey, $arrayValue, \PDO::PARAM_STR); $bindIndex++; break; case Operator::TYPE_ARRAY_REMOVE: $value = $values[0] ?? null; $bindKey = "op_{$bindIndex}"; // Always JSON encode for PostgreSQL jsonb comparison $stmt->bindValue(':' . $bindKey, json_encode($value), \PDO::PARAM_STR); $bindIndex++; break; case Operator::TYPE_ARRAY_INTERSECT: case Operator::TYPE_ARRAY_DIFF: + if (\count($values) > self::MAX_ARRAY_OPERATOR_SIZE) { + throw new DatabaseException("Array size " . \count($values) . " exceeds maximum allowed size of " . self::MAX_ARRAY_OPERATOR_SIZE . " for array operations"); + } $arrayValue = json_encode($values); $bindKey = "op_{$bindIndex}"; $stmt->bindValue(':' . $bindKey, $arrayValue, \PDO::PARAM_STR); $bindIndex++; break;src/Database/Adapter/MariaDB.php (1)
1962-1988: Guard every numeric operator against divide-by-zero
TYPE_MULTIPLY,TYPE_DIVIDE, andTYPE_MODULOall divide by:$bindKey(and the guard branches divide by the same placeholder) without aNULLIF. Supplying zero instantly triggersSQLSTATE[22012]and aborts the update. The SQL path must be resilient to user input.Please wrap every divisor in
NULLIF(:$bindKey, 0)(and the equivalent guard checks) so a zero operand yieldsNULLinstead of crashing:case Operator::TYPE_MULTIPLY: $bindKey = "op_{$bindIndex}"; $bindIndex++; if (isset($values[1])) { $maxKey = "op_{$bindIndex}"; $bindIndex++; return "{$quotedColumn} = CASE WHEN COALESCE({$quotedColumn}, 0) >= :$maxKey THEN :$maxKey - WHEN :$bindKey > 0 AND COALESCE({$quotedColumn}, 0) > :$maxKey / :$bindKey THEN :$maxKey - WHEN :$bindKey < 0 AND COALESCE({$quotedColumn}, 0) < :$maxKey / :$bindKey THEN :$maxKey + WHEN :$bindKey > 0 AND COALESCE({$quotedColumn}, 0) > :$maxKey / NULLIF(:$bindKey, 0) THEN :$maxKey + WHEN :$bindKey < 0 AND COALESCE({$quotedColumn}, 0) < :$maxKey / NULLIF(:$bindKey, 0) THEN :$maxKey ELSE COALESCE({$quotedColumn}, 0) * :$bindKey END"; } return "{$quotedColumn} = COALESCE({$quotedColumn}, 0) * :$bindKey"; case Operator::TYPE_DIVIDE: $bindKey = "op_{$bindIndex}"; $bindIndex++; if (isset($values[1])) { $minKey = "op_{$bindIndex}"; $bindIndex++; return "{$quotedColumn} = CASE - WHEN :$bindKey != 0 AND COALESCE({$quotedColumn}, 0) / :$bindKey <= :$minKey THEN :$minKey - ELSE COALESCE({$quotedColumn}, 0) / :$bindKey + WHEN :$bindKey != 0 AND COALESCE({$quotedColumn}, 0) / NULLIF(:$bindKey, 0) <= :$minKey THEN :$minKey + ELSE COALESCE({$quotedColumn}, 0) / NULLIF(:$bindKey, 0) END"; } - return "{$quotedColumn} = COALESCE({$quotedColumn}, 0) / :$bindKey"; + return "{$quotedColumn} = COALESCE({$quotedColumn}, 0) / NULLIF(:$bindKey, 0)"; case Operator::TYPE_MODULO: $bindKey = "op_{$bindIndex}"; $bindIndex++; - return "{$quotedColumn} = MOD(COALESCE({$quotedColumn}, 0), :$bindKey)"; + return "{$quotedColumn} = MOD(COALESCE({$quotedColumn}, 0), NULLIF(:$bindKey, 0))";src/Database/Adapter/SQLite.php (2)
1298-1316: Consider distinguishing _uid unique violations from other unique constraints.The current code maps all UNIQUE constraint violations to
DuplicateException. However, based on learnings from the MariaDB adapter, only duplicate_uidviolations should throwDuplicateException, while other unique constraint violations (including PRIMARY key and user-defined unique constraints) should throwUniqueException.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.
811-824: Critical: Add null check for getOperatorSQL return value to prevent malformed SQL.The method
getOperatorSQL()has return type?stringand can return null (line 1465). At line 812, the result is used without a null check, then a comma is unconditionally appended at line 824. This creates invalid SQL if the operator SQL is null.The inconsistency is evident:
getUpsertStatement()(line 1831) properly checksif ($operatorSQL !== null)before using the value. Apply the same pattern here:// Check if this is an operator, spatial attribute, or regular attribute if (isset($operators[$attribute])) { $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $opIndex); - $columns .= $operatorSQL; + if ($operatorSQL !== null) { + $columns .= $operatorSQL; + $columns .= ','; + } } elseif ($this->getSupportForSpatialAttributes() && \in_array($attribute, $spatialAttributes, true)) { $bindKey = 'key_' . $keyIndex; $columns .= "`{$column}` = " . $this->getSpatialGeomFromText(':' . $bindKey); + $columns .= ','; $keyIndex++; } else { $bindKey = 'key_' . $keyIndex; $columns .= "`{$column}`" . '=:' . $bindKey; + $columns .= ','; $keyIndex++; } - -$columns .= ',';🤖 Prompt for AI Agents
In src/Database/Adapter/SQLite.php around lines 807 to 828, the result of getOperatorSQL() can be null but is used unconditionally at line 813, which can produce malformed SQL; update the operator handling to call getOperatorSQL and store the result, then only append the result and the trailing comma when the result is not null (mirror the null-check pattern used in getUpsertStatement at line 1831); move the comma append (line 824) inside each conditional branch so commas are only added for non-null values.
🧹 Nitpick comments (8)
tests/unit/OperatorTest.php (6)
365-371: Stabilize assertion against dynamic exception text.Use a regex match to avoid brittleness if the implementation appends type details.
- $this->expectException(OperatorException::class); - $this->expectExceptionMessage('Invalid operator. Must be an array'); - Operator::parse('"string"'); + $this->expectException(OperatorException::class); + $this->expectExceptionMessageMatches('/^Invalid operator\. Must be an array\b/'); + Operator::parse('"string"');
404-413: Close the resource handle to avoid leaks.Minor hygiene: ensure the memory stream is closed even when the exception is thrown.
- $operator = new Operator(Operator::TYPE_INCREMENT, 'test', []); - $operator->setValues([fopen('php://memory', 'r')]); // Resource can't be JSON encoded - - $this->expectException(OperatorException::class); - $this->expectExceptionMessage('Invalid Json'); - $operator->toString(); + $operator = new Operator(Operator::TYPE_INCREMENT, 'test', []); + $h = fopen('php://memory', 'r'); // Resource can't be JSON encoded + $operator->setValues([$h]); + + $this->expectException(OperatorException::class); + $this->expectExceptionMessage('Invalid Json'); + try { + $operator->toString(); + } finally { + fclose($h); + }
81-86: Remove duplicate concat helper block.This repeats the earlier concat assertions and adds no new coverage. Trim to reduce noise.
- // Test concat helper - $operator = Operator::stringConcat(' - Updated'); - $this->assertEquals(Operator::TYPE_STRING_CONCAT, $operator->getMethod()); - $this->assertEquals('', $operator->getAttribute()); - $this->assertEquals([' - Updated'], $operator->getValues());
157-161: Drop redundant concat assertions.Duplicate of previous concat checks; safe to remove.
- $concatOp = Operator::stringConcat('suffix'); // Deprecated - $this->assertFalse($concatOp->isNumericOperation()); - $this->assertFalse($concatOp->isArrayOperation()); - $this->assertTrue($concatOp->isStringOperation());
330-343: Add a deep-clone assertion with nested Operator in values.Current clone test covers shallow fields. Also verify that nested Operator instances in values are cloned, not shared.
$operator1 = Operator::increment(5); $operator2 = clone $operator1; @@ $this->assertEquals(Operator::TYPE_DECREMENT, $operator2->getMethod()); + + // Deep-clone check: nested Operator in values should be cloned + $parent = new Operator(Operator::TYPE_ARRAY_APPEND, 'tags', [Operator::stringConcat('x')]); + $cloned = clone $parent; + $this->assertNotSame($parent->getValues()[0], $cloned->getValues()[0], 'Nested Operator should be cloned by __clone');
828-835: Add coverage for dateAddDays/dateSubDays helpers.These helpers exist but aren’t exercised; add a small test to prevent regressions.
public function testDateArithmeticHelpers(): void { $add = Operator::dateAddDays(3); $this->assertEquals(Operator::TYPE_DATE_ADD_DAYS, $add->getMethod()); $this->assertEquals([3], $add->getValues()); $sub = Operator::dateSubDays(2); $this->assertEquals(Operator::TYPE_DATE_SUB_DAYS, $sub->getMethod()); $this->assertEquals([2], $sub->getValues()); }bin/tasks/operators.php (2)
569-575: Remove unused parameter$isBulk.The
$isBulkparameter is declared but never used in the function body. This creates confusion about the function's behavior.Apply this diff:
function benchmarkOperation( Database $database, int $iterations, string $operation, - bool $isBulk, bool $useOperators ): array {And update the call sites at lines 207-269 to remove the
falseargument.
608-621: Remove unused variable$docor use it for explicit read-modify-write.Line 617 fetches the document but never uses it. If the intent was to simulate a read-modify-write pattern, the document should be used to compute the new counter value. Otherwise, remove the unused fetch.
If simulating read-modify-write (recommended for fair comparison):
} else { // Without operators, we need to read and update individually // because updateDocuments with queries would apply the same value to all matching docs $doc = $database->getDocument('operators_test', $docId); $database->updateDocument('operators_test', $docId, new Document([ - 'counter' => $i + 1 + 'counter' => $doc->getAttribute('counter', 0) + 1 ])); }Or if not needed, remove line 617.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
bin/tasks/operators.php(1 hunks)src/Database/Adapter/MariaDB.php(10 hunks)src/Database/Adapter/Mongo.php(2 hunks)src/Database/Adapter/Postgres.php(11 hunks)src/Database/Adapter/SQL.php(9 hunks)src/Database/Adapter/SQLite.php(10 hunks)src/Database/Operator.php(1 hunks)src/Database/Validator/Operator.php(1 hunks)tests/e2e/Adapter/Scopes/DocumentTests.php(3 hunks)tests/unit/OperatorTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#613
File: src/Database/Adapter/Postgres.php:1254-1319
Timestamp: 2025-07-01T11:31:37.438Z
Learning: In PostgreSQL adapter methods like getUpsertStatement, complexity for database-specific SQL generation is acceptable when the main business logic is properly separated in the parent SQL adapter class, following the adapter pattern where each database adapter handles its own SQL syntax requirements.
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Adapter/Mongo.phpsrc/Database/Adapter/MariaDB.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/Postgres.phpsrc/Database/Adapter/SQL.phpsrc/Database/Adapter/SQLite.php
📚 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.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/Postgres.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:
src/Database/Adapter/MariaDB.phptests/e2e/Adapter/Scopes/DocumentTests.phpsrc/Database/Adapter/SQL.phpsrc/Database/Adapter/SQLite.php
📚 Learning: 2025-07-01T11:31:37.438Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#613
File: src/Database/Adapter/Postgres.php:1254-1319
Timestamp: 2025-07-01T11:31:37.438Z
Learning: In PostgreSQL adapter methods like getUpsertStatement, complexity for database-specific SQL generation is acceptable when the main business logic is properly separated in the parent SQL adapter class, following the adapter pattern where each database adapter handles its own SQL syntax requirements.
Applied to files:
tests/e2e/Adapter/Scopes/DocumentTests.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Validator/Operator.php
🧬 Code graph analysis (9)
src/Database/Adapter/Mongo.php (5)
src/Database/Adapter/SQL.php (1)
getSupportForOperators(1566-1569)src/Database/Adapter/Pool.php (2)
getSupportForOperators(403-406)getSupportForIntegerBooleans(618-621)src/Database/Adapter.php (2)
getSupportForOperators(1085-1085)getSupportForIntegerBooleans(1413-1413)src/Database/Adapter/MariaDB.php (1)
getSupportForIntegerBooleans(1774-1777)src/Database/Adapter/Postgres.php (1)
getSupportForIntegerBooleans(2000-2003)
tests/unit/OperatorTest.php (3)
src/Database/Exception.php (1)
Exception(7-21)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)stringConcat(486-489)stringReplace(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/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(1239-1248)getSupportForIntegerBooleans(1413-1413)quote(467-467)src/Database/Adapter/Postgres.php (5)
getOperatorSQL(2408-2612)bindOperatorParams(2623-2658)getPDOType(1895-1904)getSupportForIntegerBooleans(2000-2003)quote(2087-2090)src/Database/Adapter/SQL.php (5)
getOperatorSQL(1873-1873)getSpatialGeomFromText(1668-1680)bindOperatorParams(1883-2045)convertArrayToWKT(2900-2939)getPDOType(2159-2159)src/Database/Adapter/SQLite.php (3)
getOperatorSQL(1465-1773)getSpatialGeomFromText(1023-1026)bindOperatorParams(1411-1444)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
src/Database/Adapter/Postgres.php (6)
src/Database/Database.php (1)
Database(38-8559)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)
getSpatialAttributes(436-449)getOperatorSQL(1873-1873)getSpatialGeomFromText(1668-1680)bindOperatorParams(1883-2045)convertArrayToWKT(2900-2939)getPDOType(2159-2159)src/Database/Adapter.php (3)
filter(1239-1248)getSupportForIntegerBooleans(1413-1413)quote(467-467)src/Database/Adapter/MariaDB.php (4)
getOperatorSQL(1920-2120)getPDOType(1713-1721)getSupportForIntegerBooleans(1774-1777)quote(1906-1909)src/Database/Adapter/SQLite.php (3)
getOperatorSQL(1465-1773)getSpatialGeomFromText(1023-1026)bindOperatorParams(1411-1444)
src/Database/Adapter/SQL.php (5)
src/Database/Operator.php (6)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)getAttribute(147-150)extractOperators(675-696)src/Database/Adapter.php (4)
Adapter(16-1415)filter(1239-1248)getSupportForOperators(1085-1085)getSupportForIntegerBooleans(1413-1413)src/Database/Adapter/MariaDB.php (4)
getOperatorSQL(1920-2120)getPDOType(1713-1721)getSupportForIntegerBooleans(1774-1777)getUpsertStatement(1214-1293)src/Database/Adapter/Postgres.php (6)
getOperatorSQL(2408-2612)bindOperatorParams(2623-2658)Postgres(30-2659)getPDOType(1895-1904)getSupportForIntegerBooleans(2000-2003)getUpsertStatement(1336-1415)src/Database/Adapter/SQLite.php (3)
getOperatorSQL(1465-1773)bindOperatorParams(1411-1444)getUpsertStatement(1787-1867)
src/Database/Validator/Operator.php (3)
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/Document.php (1)
getId(63-66)
src/Database/Adapter/SQLite.php (6)
src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)increment(409-416)src/Database/Adapter/MariaDB.php (7)
processException(1838-1904)updateDocument(951-1201)getOperatorSQL(1920-2120)getSupportForSpatialAttributes(2132-2135)getPDOType(1713-1721)quote(1906-1909)getUpsertStatement(1214-1293)src/Database/Adapter/Postgres.php (8)
processException(2038-2081)updateDocument(1092-1324)getOperatorSQL(2408-2612)getSupportForSpatialAttributes(2097-2100)bindOperatorParams(2623-2658)getPDOType(1895-1904)quote(2087-2090)getUpsertStatement(1336-1415)src/Database/Adapter/SQL.php (10)
processException(2385-2388)getOperatorSQL(1873-1873)getSupportForSpatialAttributes(1546-1549)getSpatialGeomFromText(1668-1680)bindOperatorParams(1883-2045)getPDOType(2159-2159)getPDO(2147-2150)count(3175-3237)getUpsertStatement(1702-1710)getSQLTable(1859-1862)src/Database/Adapter/MySQL.php (2)
processException(148-165)getOperatorSQL(278-306)src/Database/Adapter.php (6)
updateDocument(725-725)filter(1239-1248)getSupportForSpatialAttributes(1071-1071)getNamespace(107-110)count(825-825)quote(467-467)
src/Database/Operator.php (3)
src/Database/Database.php (2)
Database(38-8559)__construct(423-621)src/Database/Validator/Operator.php (2)
Operator(10-383)__construct(29-37)src/Database/Exception/Operator.php (1)
Operator(7-9)
bin/tasks/operators.php (12)
src/Database/Adapter.php (1)
Adapter(16-1415)src/Database/Database.php (6)
Database(38-8559)create(1330-1349)updateDocument(4925-5163)updateDocuments(5186-5410)upsertDocument(5813-5833)upsertDocuments(5847-5862)src/Database/Adapter/MariaDB.php (4)
MariaDB(21-2222)create(31-46)updateDocument(951-1201)getSupportForUpserts(1789-1792)src/Database/Adapter/MySQL.php (1)
MySQL(14-307)src/Database/Adapter/Postgres.php (4)
Postgres(30-2659)create(137-165)updateDocument(1092-1324)getSupportForUpserts(2015-2018)src/Database/Adapter/SQLite.php (4)
SQLite(34-1868)create(115-118)updateDocument(643-886)getSupportForUpserts(980-983)src/Database/Operator.php (7)
Operator(14-698)increment(409-416)decrement(425-432)multiply(510-517)stringConcat(486-489)toggle(544-547)arrayAppend(441-444)src/Database/Validator/Operator.php (1)
Operator(10-383)src/Database/PDO.php (1)
PDO(13-143)src/Database/Query.php (2)
Query(8-1169)equal(452-455)src/Database/Validator/Authorization.php (1)
Authorization(7-225)src/Database/Document.php (1)
getId(63-66)
🪛 PHPMD (2.15.0)
src/Database/Adapter/SQLite.php
1023-1023: Avoid unused parameters such as '$srid'. (undefined)
(UnusedFormalParameter)
bin/tasks/operators.php
573-573: Avoid unused parameters such as '$isBulk'. (undefined)
(UnusedFormalParameter)
617-617: Avoid unused local variables such as '$doc'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (9)
src/Database/Adapter/Mongo.php (3)
2892-2900: LGTM! Correct capability flag for MongoDB.MongoDB correctly returns
falsefor operator support since it uses its own atomic update operators ($inc,$set, etc.) rather than the SQL-based operator framework being introduced in this PR.
2902-2910: LGTM! Correct capability flag for MongoDB.MongoDB correctly returns
falsesince it has native boolean type support and doesn't require conversion to integers (0/1), similar to PostgreSQL.
1497-1503: Code change is correct—parameter order matches the method signature.The
Client::update()method signature from utopia-php/mongo v0.11.0 is:public function update(string $collection, array $where = [], array $updates = [], array $options = [], bool $multi = false): intThe call at lines 1497-1502 correctly maps parameters:
$name(collection),$filters(where),$updateQuery(updates),$options(4th position), andmulti: true(5th position). Using positional syntax for$optionsinstead of named syntax is valid and safe—both forms refer to the same parameter. This also aligns with the existing positional call at line 1453.tests/e2e/Adapter/Scopes/DocumentTests.php (3)
5810-5813: Good guard for adapters without upsert support.Early-exit with expectNotToPerformAssertions keeps the suite green on unsupported backends.
6076-6080: Consistent skip pattern for update count test.Matches the upsert capability gating used elsewhere.
9-11: Import looks fine.No issues spotted with adding Utopia\Database\DateTime here.
src/Database/Operator.php (1)
527-537: Use strict equality in divide-by-zero guard.Line 529 uses loose equality
== 0, which allows type coercion. For example,"0.0"or even"0abc"could incorrectly match due to PHP's type juggling, potentially allowing division by values that should be rejected.Apply this diff:
public static function divide(int|float $divisor, int|float|null $min = null): self { - if ($divisor == 0) { + if ($divisor === 0 || $divisor === 0.0) { throw new OperatorException('Division by zero is not allowed'); }Likely an incorrect or invalid review comment.
src/Database/Adapter/SQLite.php (2)
1015-1026: PHPMD unused parameter warning is a false positive.The static analysis tool flags
$sridas unused at line 1023. This is expected because the method overrides the parent signature to return the placeholder unchanged—SQLite doesn't supportST_GeomFromText, so the SRID parameter is intentionally ignored.No action required.
1375-1773: Well-implemented SQLite operator support with appropriate adaptations.The operator implementation demonstrates strong engineering:
- Runtime capability detection (lines 1375-1400):
getSupportForMathFunctions()gracefully checks POWER availability rather than assuming SQLite compilation flags- Comprehensive operator coverage (lines 1465-1773): Handles numeric, string, boolean, array, and date operators with SQLite-appropriate SQL
- Array operator implementation: Proper use of
json_each()andjson_group_array()for JSON array manipulation, with documented type conversion limitations (lines 1449-1458)- ARRAY_FILTER completeness (lines 1682-1750): Includes
isNull/isNotNullcases and numeric casting for comparisons as required by past reviews- Bind parameter discipline (lines 1411-1444): Correctly manages bind index increments across SQL generation and binding phases
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Database/Database.php (3)
5137-5156: Avoid double-decoding and redundant casting after refetch (operator path).Refetched docs from refetchDocuments() are already castingAfter+decoded via find(). Decoding again here can cause type errors for filters expecting strings (e.g., spatial decode closures accept ?string) and adds overhead. Track operator usage and skip the second decode/casting.
- $document = $this->withTransaction(function () use ($collection, $id, $document, $newUpdatedAt) { + $usedOperators = false; + $document = $this->withTransaction(function () use ($collection, $id, $document, $newUpdatedAt, &$usedOperators) { @@ - // If operators were used, refetch document to get computed values - if (!empty($operators)) { + // If operators were used, refetch document to get computed values + if (!empty($operators)) { + $usedOperators = true; $refetched = $this->refetchDocuments($collection, [$document]); $document = $refetched[0]; } @@ - if (!$this->inBatchRelationshipPopulation && $this->resolveRelationships) { + if (!$this->inBatchRelationshipPopulation && $this->resolveRelationships) { $documents = $this->silent(fn () => $this->populateDocumentsRelationships([$document], $collection, $this->relationshipFetchDepth)); $document = $documents[0]; } - - $document = $this->decode($collection, $document); + // Skip decode when refetched (already decoded by find()) + if (!$usedOperators) { + $document = $this->decode($collection, $document); + }
5372-5381: Skip castingAfter/decode on refetched batch (operator path).After refetchDocuments(), each doc has already been adapter-casted and decoded by find(). Re-applying castingAfter + decode is redundant and risks type errors on typed filters.
- if (!empty($operators)) { - $batch = $this->refetchDocuments($collection, $batch); - } + $refetched = !empty($operators); + if ($refetched) { + $batch = $this->refetchDocuments($collection, $batch); + } @@ - foreach ($batch as $index => $doc) { - $doc = $this->adapter->castingAfter($collection, $doc); + foreach ($batch as $index => $doc) { + if (!$refetched) { + $doc = $this->adapter->castingAfter($collection, $doc); + } $doc->removeAttribute('$skipPermissionsUpdate'); $this->purgeCachedDocument($collection->getId(), $doc->getId()); - $doc = $this->decode($collection, $doc); + if (!$refetched) { + $doc = $this->decode($collection, $doc); + }Also applies to: 5376-5381
5350-5357: Use pre-update timestamp for conflict check (logic bug).You’re comparing the request timestamp to the NEW $updatedAt (post-merge). This can falsely trigger conflicts. Capture the original updatedAt before merging and compare against that.
- $document->setAttribute('$skipPermissionsUpdate', $skipPermissionsUpdate); - - $new = new Document(\array_merge($document->getArrayCopy(), $updates->getArrayCopy())); + $document->setAttribute('$skipPermissionsUpdate', $skipPermissionsUpdate); + $prevUpdatedAt = $document->getUpdatedAt(); + $new = new Document(\array_merge($document->getArrayCopy(), $updates->getArrayCopy())); @@ - // Check if document was updated after the request timestamp - try { - $oldUpdatedAt = new \DateTime($document->getUpdatedAt()); + // Check if document was updated after the request timestamp (use pre-merge value) + try { + $oldUpdatedAt = new \DateTime($prevUpdatedAt);
♻️ Duplicate comments (4)
src/Database/Adapter/SQL.php (1)
2113-2114: ARRAY_FILTER doesn't filter in PHP-side application.When a new document contains only operators (no regular updates),
applyOperatorToValueis called. For TYPE_ARRAY_FILTER, it returns the original array without filtering, meaning operator-only upserts won't apply the filter. This diverges from SQL semantics.This issue was already flagged in past reviews but appears unaddressed.
src/Database/Adapter/MariaDB.php (1)
1966-1982: Division-by-zero can still occur in DIVIDE and MODULO.Lines 1973-1974 and 1981 perform division/modulo without guarding against zero divisors. When the bound value is 0, these will raise database errors.
Apply NULLIF to divisors:
- WHEN :$bindKey != 0 AND COALESCE({$quotedColumn}, 0) / :$bindKey <= :$minKey THEN :$minKey - ELSE COALESCE({$quotedColumn}, 0) / :$bindKey + WHEN :$bindKey != 0 AND COALESCE({$quotedColumn}, 0) / NULLIF(:$bindKey, 0) <= :$minKey THEN :$minKey + ELSE COALESCE({$quotedColumn}, 0) / NULLIF(:$bindKey, 0) @@ - return "{$quotedColumn} = MOD(COALESCE({$quotedColumn}, 0), :$bindKey)"; + return "{$quotedColumn} = MOD(COALESCE({$quotedColumn}, 0), NULLIF(:$bindKey, 0))";Additionally, MULTIPLY guards (lines 1959-1960) divide by :$bindKey to check limits, which also fails when :$bindKey is 0.
This issue was flagged in previous reviews but remains unaddressed.
src/Database/Adapter/SQLite.php (2)
1298-1316: 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_uidviolations should throwDuplicateException, while other unique constraint violations (including PRIMARY key and user-defined unique constraints) should throwUniqueException.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:if ( ($e->getCode() === 'HY000' && isset($e->errorInfo[1]) && ($e->errorInfo[1] === 1 || $e->errorInfo[1] === 19)) || $e->getCode() === '23000' ) { $message = $e->getMessage(); if ( (isset($e->errorInfo[1]) && $e->errorInfo[1] === 19) || $e->getCode() === '23000' || stripos($message, 'unique') !== false || stripos($message, 'duplicate') !== false ) { // Check if it's specifically a _uid constraint violation if (stripos($message, '_uid') !== false) { return new DuplicateException('Document already exists', $e->getCode(), $e); } // Other unique constraint violations should throw UniqueException return new UniqueException('Unique constraint violation: ' . $message, $e->getCode(), $e); } }Note: This will require importing
UniqueException.Based on learnings.
811-824: Add null check for getOperatorSQL return value (inconsistent with getUpsertStatement).The
getOperatorSQL()method has return type?stringand can return null. InupdateDocument(lines 812-813), the result is appended to$columnswithout a null check, then a comma is unconditionally added (line 824). This can produce malformed SQL if the operator SQL is null.However,
getUpsertStatement(lines 1830-1833) correctly checksif ($operatorSQL !== null)before using the value. Apply the same null check here for consistency:if (isset($operators[$attribute])) { $operatorSQL = $this->getOperatorSQL($column, $operators[$attribute], $opIndex); - $columns .= $operatorSQL; + if ($operatorSQL !== null) { + $columns .= $operatorSQL; + $columns .= ','; + } } elseif ($this->getSupportForSpatialAttributes() && \in_array($attribute, $spatialAttributes, true)) { $bindKey = 'key_' . $keyIndex; $columns .= "`{$column}` = " . $this->getSpatialGeomFromText(':' . $bindKey); + $columns .= ','; $keyIndex++; } else { $bindKey = 'key_' . $keyIndex; $columns .= "`{$column}`" . '=:' . $bindKey; + $columns .= ','; $keyIndex++; } - -$columns .= ',';Note: A past review flagged this same issue and marked it as addressed, but the code still shows the inconsistency. This may be a regression.
🧹 Nitpick comments (8)
tests/unit/OperatorTest.php (4)
30-116: Consider refactoring to use PHPUnit data providers.This test method is quite long and follows repetitive patterns. You could improve maintainability by using a data provider to test all helper methods with the same assertion pattern.
Example refactor:
/** * @dataProvider helperMethodsProvider */ public function testHelperMethods(callable $factory, string $expectedMethod, array $expectedValues, string $expectedAttribute = ''): void { $operator = $factory(); $this->assertEquals($expectedMethod, $operator->getMethod()); $this->assertEquals($expectedAttribute, $operator->getAttribute()); $this->assertEquals($expectedValues, $operator->getValues()); } public function helperMethodsProvider(): array { return [ 'increment with value' => [fn() => Operator::increment(5), Operator::TYPE_INCREMENT, [5]], 'increment default' => [fn() => Operator::increment(), Operator::TYPE_INCREMENT, [1]], 'decrement' => [fn() => Operator::decrement(1), Operator::TYPE_DECREMENT, [1]], // ... more cases ]; }
200-204: Remove duplicate assertion.Line 203 duplicates the check for
TYPE_STRING_CONCATthat was already tested on line 200.Apply this diff:
$this->assertTrue(Operator::isMethod(Operator::TYPE_STRING_CONCAT)); $this->assertTrue(Operator::isMethod(Operator::TYPE_STRING_REPLACE)); $this->assertTrue(Operator::isMethod(Operator::TYPE_TOGGLE)); - $this->assertTrue(Operator::isMethod(Operator::TYPE_STRING_CONCAT)); $this->assertTrue(Operator::isMethod(Operator::TYPE_DATE_SET_NOW));
690-695: Remove duplicate assertion.Line 694 duplicates the check for
TYPE_STRING_CONCATthat was already tested on line 690 for the 'concat' operator.Apply this diff:
$this->assertEquals(Operator::TYPE_STRING_REPLACE, $operators['replace']->getMethod()); $this->assertEquals(Operator::TYPE_TOGGLE, $operators['toggle']->getMethod()); $this->assertEquals(Operator::TYPE_DATE_SET_NOW, $operators['dateSetNow']->getMethod()); - $this->assertEquals(Operator::TYPE_STRING_CONCAT, $operators['concat']->getMethod()); $this->assertEquals(Operator::TYPE_MODULO, $operators['modulo']->getMethod());
838-838: Remove or clarify confusing comment.The comment
// Deprecatedon line 838 next to theTYPE_STRING_CONCATtest case is misleading. This operator type is not deprecated and is actively tested throughout the file. If this was meant to indicate something else, please clarify; otherwise, remove the comment.Apply this diff:
$operators = [ ['method' => Operator::TYPE_STRING_CONCAT, 'attribute' => 'title', 'values' => [' - Updated']], - ['method' => Operator::TYPE_STRING_CONCAT, 'attribute' => 'subtitle', 'values' => [' - Updated']], // Deprecated + ['method' => Operator::TYPE_STRING_CONCAT, 'attribute' => 'subtitle', 'values' => [' - Updated']], ['method' => Operator::TYPE_STRING_REPLACE, 'attribute' => 'content', 'values' => ['old', 'new']],src/Database/Database.php (4)
6136-6154: Avoid re-casting decoded docs in upsert operator batches.When hasOperators is true and the batch was refetched, docs are already castingAfter+decoded. Re-running castingAfter risks double-casting; keep decode/casting only for non-operator batches.
- // Check if any document in the batch contains operators + // Check if any document in the batch contains operators $hasOperators = false; foreach ($batch as $doc) { $extracted = Operator::extractOperators($doc->getArrayCopy()); if (!empty($extracted['operators'])) { $hasOperators = true; break; } } if ($hasOperators) { $batch = $this->refetchDocuments($collection, $batch); } foreach ($batch as $index => $doc) { - $doc = $this->adapter->castingAfter($collection, $doc); - if (!$hasOperators) { - $doc = $this->decode($collection, $doc); - } + if (!$hasOperators) { + $doc = $this->adapter->castingAfter($collection, $doc); + $doc = $this->decode($collection, $doc); + }
736-739: Refetch without relationship population to avoid duplicate work.refetchDocuments() calls find() which may populate relationships, and you populate again later. Wrap in skipRelationships() to refetch only computed scalar values; relationships will be populated once afterward.
- // Fetch fresh copies with computed operator values - $refetched = Authorization::skip(fn () => $this->silent( - fn () => $this->find($collection->getId(), [Query::equal('$id', $docIds)]) - )); + // Fetch fresh copies with computed operator values (skip relationship population) + $refetched = Authorization::skip(fn () => $this->skipRelationships(fn () => $this->silent( + fn () => $this->find($collection->getId(), [Query::equal('$id', $docIds)]) + )));Also applies to: 721-753
4962-4966: Drop unused foreach key variable to silence PHPMD and reduce noise.The $attribute key is unused in both loops.
- foreach ($operators as $attribute => $operator) { + foreach ($operators as $operator) { if (!$operatorValidator->isValid($operator)) { throw new StructureException($operatorValidator->getDescription()); } }- foreach ($operators as $attribute => $operator) { + foreach ($operators as $operator) { if (!$operatorValidator->isValid($operator)) { throw new StructureException($operatorValidator->getDescription()); } }Also applies to: 5318-5323
5370-5374: Nit: $updates castingBefore result is unused.$updates = $this->adapter->castingBefore($collection, $updates); is not used later. Safe to remove.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter/MariaDB.php(10 hunks)src/Database/Adapter/SQL.php(8 hunks)src/Database/Adapter/SQLite.php(11 hunks)src/Database/Database.php(20 hunks)tests/unit/OperatorTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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:
src/Database/Adapter/SQL.phpsrc/Database/Database.phpsrc/Database/Adapter/MariaDB.phpsrc/Database/Adapter/SQLite.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Adapter/SQL.phpsrc/Database/Database.phpsrc/Database/Adapter/MariaDB.phpsrc/Database/Adapter/SQLite.php
📚 Learning: 2025-07-30T19:17:53.630Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#642
File: src/Database/Validator/PartialStructure.php:43-52
Timestamp: 2025-07-30T19:17:53.630Z
Learning: In PartialStructure validator, when filtering for required attributes validation using the $requiredAttributes parameter, $this->attributes should be used instead of the merged $attributes array because this validation is specifically for internal attributes like $createdAt and $updatedAt that are defined in the base Structure class, not collection-specific attributes.
Applied to files:
src/Database/Database.php
📚 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 (5)
tests/unit/OperatorTest.php (3)
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)stringConcat(486-489)stringReplace(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/Validator/Operator.php (1)
Operator(10-383)src/Database/Exception/Operator.php (1)
Operator(7-9)
src/Database/Adapter/SQL.php (8)
src/Database/Database.php (3)
Database(38-8547)count(7309-7360)getTenant(1141-1144)src/Database/Operator.php (6)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)getAttribute(147-150)extractOperators(675-696)src/Database/Adapter.php (8)
Adapter(16-1415)filter(1239-1248)quote(467-467)execute(1336-1336)getSupportForOperators(1085-1085)count(825-825)getTenant(216-219)getSupportForIntegerBooleans(1413-1413)src/Database/Adapter/MariaDB.php (6)
getOperatorSQL(1915-2115)quote(1901-1904)getPDOType(1708-1716)processException(1833-1899)getSupportForIntegerBooleans(1769-1772)getUpsertStatement(1214-1288)src/Database/Adapter/SQLite.php (5)
getOperatorSQL(1465-1773)getSpatialGeomFromText(1023-1026)bindOperatorParams(1411-1444)processException(1291-1324)getUpsertStatement(1787-1867)src/Database/Adapter/Postgres.php (9)
getOperatorSQL(2408-2612)quote(2087-2090)bindOperatorParams(2623-2658)Postgres(30-2659)getPDOType(1895-1904)execute(86-106)processException(2038-2081)getSupportForIntegerBooleans(2000-2003)getUpsertStatement(1336-1415)src/Database/Adapter/MySQL.php (2)
getOperatorSQL(278-306)processException(148-165)src/Database/Adapter/Pool.php (5)
quote(118-121)execute(518-521)getSupportForOperators(403-406)count(273-276)getSupportForIntegerBooleans(618-621)
src/Database/Database.php (9)
src/Database/Operator.php (4)
Operator(14-698)extractOperators(675-696)getAttribute(147-150)setAttribute(188-193)src/Database/Validator/Operator.php (3)
Operator(10-383)isValid(60-85)getDescription(46-49)src/Database/Validator/Queries/Document.php (1)
Document(10-44)src/Database/Document.php (10)
Document(12-470)getId(63-66)find(304-322)getArrayCopy(423-458)getAttribute(224-231)getCreatedAt(162-165)getTenant(178-187)setAttribute(244-261)getAttributes(194-212)isEmpty(396-399)src/Database/Mirror.php (2)
silent(172-175)updateDocument(658-704)src/Database/Adapter/SQL.php (4)
find(2955-3160)count(3172-3234)castingAfter(1637-1640)getSupportForAttributes(948-951)src/Database/Adapter/Mongo.php (6)
find(1859-2021)count(2093-2180)updateDocument(1432-1459)castingAfter(1194-1269)getMinDateTime(2589-2592)getSupportForAttributes(2645-2648)src/Database/Adapter/Pool.php (6)
find(263-266)count(273-276)updateDocument(238-241)castingAfter(593-596)getMinDateTime(318-321)getSupportForAttributes(328-331)src/Database/Validator/PartialStructure.php (2)
isValid(19-61)PartialStructure(8-62)
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.php (2)
getSupportForIntegerBooleans(1413-1413)quote(467-467)src/Database/Adapter/SQL.php (2)
getOperatorSQL(1874-1874)bindOperatorParams(1884-2045)src/Database/Adapter/MySQL.php (1)
getOperatorSQL(278-306)
src/Database/Adapter/SQLite.php (5)
src/Database/Operator.php (5)
Operator(14-698)isOperator(664-667)getMethod(139-142)getValues(155-158)increment(409-416)src/Database/Adapter/MariaDB.php (4)
processException(1833-1899)getOperatorSQL(1915-2115)getPDOType(1708-1716)getUpsertStatement(1214-1288)src/Database/Adapter/SQL.php (9)
processException(2385-2388)getSpatialAttributes(437-450)getOperatorSQL(1874-1874)getSpatialGeomFromText(1669-1681)bindOperatorParams(1884-2045)convertArrayToWKT(2897-2936)getPDOType(2159-2159)getPDO(2147-2150)getUpsertStatement(1703-1711)src/Database/Adapter/Postgres.php (5)
processException(2038-2081)getOperatorSQL(2408-2612)bindOperatorParams(2623-2658)getPDOType(1895-1904)getUpsertStatement(1336-1415)src/Database/Adapter/MySQL.php (2)
processException(148-165)getOperatorSQL(278-306)
🪛 PHPMD (2.15.0)
src/Database/Database.php
5318-5318: Avoid unused local variables such as '$attribute'. (undefined)
(UnusedLocalVariable)
src/Database/Adapter/SQLite.php
1023-1023: Avoid unused parameters such as '$srid'. (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). (14)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (23)
src/Database/Database.php (2)
86-88: MIN_INT constant addition looks good.Matches MAX_INT semantics and is useful for overflow checks.
721-753: refetchDocuments helper is a solid addition.Correctly preserves order and falls back to original docs when missing.
src/Database/Adapter/SQL.php (10)
10-10: LGTM: Imports for operator support.The DateTime and Operator imports are correctly added to support the new operator functionality.
Also applies to: 17-17
25-29: LGTM: Array size limit for operator safety.The MAX_ARRAY_OPERATOR_SIZE constant provides a reasonable safeguard against memory exhaustion during JSON_TABLE operations.
491-581: LGTM: Operator-aware updateDocuments with separate bind indices.The method correctly separates regular attributes from operators, uses independent keyIndex/opIndex counters to avoid placeholder desynchronization, handles spatial attribute WKT conversion, and preserves PostgreSQL boolean logic. The try-catch around execute provides proper error handling.
1562-1570: LGTM: Operator support capability flag.The method correctly declares operator support for SQL-based adapters.
1700-1700: LGTM: getUpsertStatement extended for operators.The signature update correctly adds optional operator support while maintaining backward compatibility.
Also applies to: 1710-1710
1865-1874: LGTM: Abstract operator SQL generation method.The abstract method design correctly delegates dialect-specific operator SQL generation to subclasses, with reference-passed bindIndex for proper placeholder tracking.
1884-2045: LGTM: Comprehensive operator parameter binding.The method correctly binds parameters for all operator types with appropriate PDO types, validates array sizes to prevent memory exhaustion, and whitelists only valid ARRAY_FILTER conditions.
2055-2112: LGTM: Safe PHP-side operator application.The method correctly applies operators in PHP for new document scenarios, with proper division-by-zero guards for DIVIDE and MODULO operators, and appropriate handling for numeric, string, boolean, date, and array operations.
2552-2888: LGTM: Sophisticated upsert with operator grouping.The method intelligently groups documents by operator signature for efficient batching, applies operators to attribute defaults for new documents, handles spatial attributes correctly, and manages permissions appropriately. The separation of operator and non-operator paths maintains clarity.
2897-2936: LGTM: Robust WKT conversion for spatial geometries.The method correctly converts array-based geometry representations to WKT format for POINT, LINESTRING, and POLYGON types, with appropriate validation at each structural level.
src/Database/Adapter/MariaDB.php (11)
11-11: LGTM: Imports for operator and exception handling.The LimitException, OperatorException, and Operator imports are correctly added to support operator functionality and enhanced error handling.
Also applies to: 13-13, 18-18
1116-1185: LGTM: Operator-aware updateDocument with correct WKT conversion.The method correctly separates operators from regular attributes using independent keyIndex/opIndex counters, converts spatial arrays to WKT format before binding, and handles boolean casting appropriately.
1210-1210: LGTM: Operator-aware upsert with deterministic binding order.The method correctly integrates operators into the upsert logic, binding operator parameters in the same attribute order used during SQL generation to maintain index alignment.
Also applies to: 1221-1221, 1239-1285
1769-1772: LGTM: Integer boolean support flag.Correctly declares that MariaDB stores boolean values as integers.
1866-1874: LGTM: Numeric out-of-range exception mapping.Correctly maps MariaDB numeric out-of-range errors (codes 1264, 1690) to LimitException for better error semantics.
1922-1997: LGTM: Numeric operators with proper limit guards.INCREMENT, DECREMENT, MULTIPLY, and POWER operators correctly implement CASE-based limit guards to prevent overflow. Note: division-by-zero guards are intentionally omitted here (handled at a different layer or by database).
1999-2010: LGTM: String operators implemented correctly.STRING_CONCAT and STRING_REPLACE use appropriate SQL functions with correct parameter binding.
2012-2014: LGTM: Boolean toggle operator.TOGGLE correctly uses NOT with COALESCE for NULL-safe boolean negation.
2016-2096: LGTM: Comprehensive array operator support.Array operators correctly use JSON_MERGE_PRESERVE, JSON_ARRAY_INSERT with JSON_EXTRACT for typed values, JSON_TABLE for filtering/aggregation, and IFNULL guards to preserve empty arrays instead of NULL. ARRAY_FILTER now includes all comparison operators (equal, notEqual, greaterThan, greaterThanEqual, lessThan, lessThanEqual, isNull, isNotNull).
2098-2110: LGTM: Date operators implemented correctly.DATE_ADD_DAYS, DATE_SUB_DAYS, and DATE_SET_NOW use appropriate SQL date functions.
2112-2114: LGTM: Unknown operator handling.Throws OperatorException for invalid operator methods.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/OperatorTest.php (3)
81-85: Remove duplicate stringConcat test.This test duplicates the stringConcat test from lines 49-52. Consider removing this duplicate to reduce maintenance overhead.
Apply this diff to remove the duplicate:
- // Test concat helper - $operator = Operator::stringConcat(' - Updated'); - $this->assertEquals(Operator::TYPE_STRING_CONCAT, $operator->getMethod()); - $this->assertEquals('', $operator->getAttribute()); - $this->assertEquals([' - Updated'], $operator->getValues()); -
694-694: Remove duplicate assertion.This assertion duplicates line 690. Both assert
TYPE_STRING_CONCATfor$operators['concat'].Apply this diff:
- $this->assertEquals(Operator::TYPE_STRING_CONCAT, $operators['concat']->getMethod());
837-838: Remove duplicate stringConcat entry from test data.The test array contains two identical stringConcat operators (lines 837 and 838), which is confusing and doesn't add test value. Consider removing one.
Apply this diff:
$operators = [ ['method' => Operator::TYPE_STRING_CONCAT, 'attribute' => 'title', 'values' => [' - Updated']], - ['method' => Operator::TYPE_STRING_CONCAT, 'attribute' => 'subtitle', 'values' => [' - Updated']], ['method' => Operator::TYPE_STRING_REPLACE, 'attribute' => 'content', 'values' => ['old', 'new']],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/OperatorTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/OperatorTest.php (4)
src/Database/Exception.php (1)
Exception(7-21)src/Database/Validator/Operator.php (1)
Operator(10-383)src/Database/Operator.php (42)
Operator(14-698)getMethod(139-142)getAttribute(147-150)getValues(155-158)getValue(164-167)increment(409-416)decrement(425-432)stringConcat(486-489)stringReplace(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)arrayUnique(619-622)arrayIntersect(630-633)arrayDiff(641-644)arrayFilter(653-656)dateAddDays(556-559)dateSubDays(567-570)src/Database/Exception/Operator.php (1)
Operator(7-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MongoDB)
🔇 Additional comments (1)
tests/unit/OperatorTest.php (1)
1-1551: Excellent comprehensive test coverage for the Operator class.This test suite provides thorough coverage of all operator types, helper methods, serialization, parsing, cloning, edge cases, and exception scenarios. The addition of tests for the six previously untested operator helper methods (arrayUnique, arrayIntersect, arrayDiff, arrayFilter, dateAddDays, dateSubDays) is particularly well-done, with each method tested for construction, type checking, serialization, parsing, and cloning.
Summary by CodeRabbit
New Features
Bug Fixes
Tests