Elevate find sum & count to SQL class #681
Conversation
|
Warning Rate limit exceeded@fogelito has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRemoved adapter-level implementations of find, count, and sum from MariaDB and Postgres, and added unified implementations of those methods to the SQL adapter, handling queries, permissions, tenant scoping, pagination, ordering, aggregations, event triggers, and parameter binding. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant SQL as SQL Adapter
participant DB as SQL Engine
participant EV as Event Dispatcher
Note over SQL: Unified implementations for find / count / sum
C->>SQL: find(queries, limit, offset, order, cursor, dir, perm)
SQL->>SQL: build WHERE (queries + permissions + tenant)
SQL->>DB: SELECT ... WHERE ... ORDER/LIMIT/OFFSET
DB-->>SQL: Rows
SQL->>SQL: map rows → Document[]
SQL->>EV: trigger DOCUMENT_FIND
SQL-->>C: Document[]
C->>SQL: count(queries, max)
SQL->>SQL: build WHERE (queries + permissions + tenant)
SQL->>DB: SELECT COUNT(*) FROM (SELECT ... LIMIT max) sub
DB-->>SQL: Count
SQL->>EV: trigger DOCUMENT_COUNT
SQL-->>C: int
C->>SQL: sum(attribute, queries, max)
SQL->>SQL: build WHERE (queries + permissions + tenant)
SQL->>DB: SELECT SUM(attribute) FROM (SELECT ... LIMIT max) sub
DB-->>SQL: Sum
SQL->>EV: trigger DOCUMENT_SUM
SQL-->>C: number
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/Database/Adapter/SQL.php (3)
2569-2572: Unify float binding across find/count/sumfind() formats doubles with configured precision; count() and sum() do not. Apply the same binding to avoid precision drift and query plan differences.
- foreach ($binds as $key => $value) { - $stmt->bindValue($key, $value, $this->getPDOType($value)); - } + foreach ($binds as $key => $value) { + if (gettype($value) === 'double') { + $stmt->bindValue($key, $this->getFloatPrecision($value), \PDO::PARAM_STR); + } else { + $stmt->bindValue($key, $value, $this->getPDOType($value)); + } + }Consider a small helper like bindParams($stmt, array $binds): void to centralize this logic.
Also applies to: 2645-2647
14-14: TimeoutException is imported and documented but never producedprocessException() currently returns the PDOException verbatim. Either (a) map driver timeout codes (e.g., HYT00) to TimeoutException in processException(), or (b) drop TimeoutException from the docblock/import.
Would you like a small processException() update that maps common timeout codes for MySQL/MariaDB and Postgres?
Also applies to: 2334-2334
2354-2368: Optional: qualify ORDER BY columns with table alias for consistencyOther predicates are qualified with $alias; ORDER BY isn’t. Not an immediate bug here (single-table), but qualifying improves clarity.
- $orders[] = "{$this->quote($attribute)} {$direction}"; + $orders[] = "{$this->quote($alias)}.{$this->quote($attribute)} {$direction}";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Database/Adapter/MariaDB.php(0 hunks)src/Database/Adapter/Postgres.php(0 hunks)src/Database/Adapter/SQL.php(2 hunks)
💤 Files with no reviewable changes (2)
- src/Database/Adapter/Postgres.php
- src/Database/Adapter/MariaDB.php
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🧬 Code graph analysis (1)
src/Database/Adapter/SQL.php (5)
src/Database/Database.php (5)
Database(37-7226)find(6188-6307)count(6404-6440)trigger(596-614)sum(6455-6484)src/Database/Exception/Timeout.php (1)
Timeout(7-9)src/Database/Query.php (4)
Query(8-1047)limit(609-612)offset(620-623)getAttribute(156-159)src/Database/Validator/Authorization.php (2)
Authorization(7-225)getRoles(101-104)src/Database/Adapter.php (8)
find(802-802)filter(1175-1184)count(825-825)getTenantQuery(1266-1266)getAttributeSelections(1151-1166)getAttributeProjection(1143-1143)trigger(450-460)sum(814-814)
🪛 GitHub Actions: CodeQL
src/Database/Adapter/SQL.php
[error] 2461-2461: Access to constant PARAM_STR on an unknown class Utopia\Database\Adapter\PDO.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
There was a problem hiding this comment.
Actionable comments posted: 0
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)
1471-1483: OR groups are internally joined with AND — breaks boolean logic.Inside the TYPE_OR branch you still glue subconditions with ' AND '. This flips OR semantics.
Apply:
- case Query::TYPE_OR: - case Query::TYPE_AND: + case Query::TYPE_OR: + case Query::TYPE_AND: $conditions = []; /* @var $q Query */ foreach ($query->getValue() as $q) { $conditions[] = $this->getSQLCondition($q, $binds, $attributes); } $method = strtoupper($query->getMethod()); - - return empty($conditions) ? '' : ' '. $method .' (' . implode(' AND ', $conditions) . ')'; + $innerGlue = $query->getMethod() === Query::TYPE_OR ? ' OR ' : ' AND '; + return empty($conditions) ? '' : ' ' . $method . ' (' . implode($innerGlue, $conditions) . ')';
890-915: Avoid string interpolation in permissions INSERT — parameterize to prevent SQL injection and quoting bugs._permissions values are injected into SQL with quotes; sanitize only removes double quotes, leaving single quotes/injection risks. Use bound parameters.
- $permissions = []; - foreach (Database::PERMISSIONS as $type) { - foreach ($document->getPermissionsByType($type) as $permission) { - $tenantBind = $this->sharedTables ? ", :_tenant" : ''; - $permission = \str_replace('"', '', $permission); - $permission = "('{$type}', '{$permission}', :_uid {$tenantBind})"; - $permissions[] = $permission; - } - } + $valuesParts = []; + $permBinds = []; + $i = 0; + foreach (Database::PERMISSIONS as $type) { + foreach ($document->getPermissionsByType($type) as $permission) { + $valuesParts[] = $this->sharedTables + ? "(:_type_{$i}, :_perm_{$i}, :_uid, :_tenant_{$i})" + : "(:_type_{$i}, :_perm_{$i}, :_uid)"; + $permBinds[":_type_{$i}"] = $type; + $permBinds[":_perm_{$i}"] = \str_replace('"', '', $permission); + if ($this->sharedTables) { + $permBinds[":_tenant_{$i}"] = $document->getTenant(); + } + $i++; + } + } - if (!empty($permissions)) { - $tenantColumn = $this->sharedTables ? ', _tenant' : ''; - $permissions = \implode(', ', $permissions); - - $sqlPermissions = " - INSERT INTO {$this->getSQLTable($name . '_perms')} (_type, _permission, _document {$tenantColumn}) - VALUES {$permissions}; - "; - - $stmtPermissions = $this->getPDO()->prepare($sqlPermissions); - $stmtPermissions->bindValue(':_uid', $document->getId()); - if ($this->sharedTables) { - $stmtPermissions->bindValue(':_tenant', $document->getTenant()); - } - } + if (!empty($valuesParts)) { + $tenantColumn = $this->sharedTables ? ', _tenant' : ''; + $columns = "(_type, _permission, _document{$tenantColumn})"; + $sqlPermissions = " + INSERT INTO {$this->getSQLTable($name . '_perms')} {$columns} + VALUES " . \implode(', ', $valuesParts) . "; + "; + $stmtPermissions = $this->getPDO()->prepare($sqlPermissions); + $stmtPermissions->bindValue(':_uid', $document->getId()); + foreach ($permBinds as $k => $v) { + $stmtPermissions->bindValue($k, $v, $this->getPDOType($v)); + } + }
🧹 Nitpick comments (1)
src/Database/Adapter/MariaDB.php (1)
1769-1796: processException likely misses common “table doesn’t exist” code (1146).Map 42S02/1146 to NotFound to improve DX; keeps existing 1051 path.
// Unknown collection - if ($e->getCode() === '42S02' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1049) { + if ($e->getCode() === '42S02' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1049) { return new NotFoundException('Collection not found', $e->getCode(), $e); } // Unknown collection // We have two of same, because docs point to 1051. // Keeping previous 1049 (above) just in case it's for older versions if ($e->getCode() === '42S02' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1051) { return new NotFoundException('Collection not found', $e->getCode(), $e); } + // Common MySQL/MariaDB "Table doesn't exist" + if ($e->getCode() === '42S02' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1146) { + return new NotFoundException('Collection not found', $e->getCode(), $e); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Postgres.php(0 hunks)src/Database/Adapter/SQL.php(2 hunks)
💤 Files with no reviewable changes (1)
- src/Database/Adapter/Postgres.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Adapter/SQL.php
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🧬 Code graph analysis (1)
src/Database/Adapter/MariaDB.php (1)
src/Database/PDO.php (1)
PDO(12-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (2)
src/Database/Adapter/MariaDB.php (2)
1635-1639: Good switch to fully-qualified PDO constants.Using \PDO::PARAM_* avoids alias collisions with Utopia\Database\PDO and ensures correct binding types.
17-18: Verify timeout flag and tenant-scoping usage in SQL::find
EnsuregetSupportForTimeouts…exists and thatfindapplies timeout and tenant-scoping internally before centralization is merged.
Summary by CodeRabbit