added spatial attribute support for the sum, count methods #670
Conversation
WalkthroughPublic API change: Adapter count and sum now accept a Document collection instead of a string ID. Implementations in MariaDB, Postgres, and Pool updated accordingly, extracting collection id and attributes for SQL condition building. Database caller updated to pass the Document. Added new spatial aggregation e2e tests (duplicated method). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant DB as Database
participant AD as Adapter (MariaDB/Postgres)
participant SQL as SQL Engine
Client->>DB: count(collectionDoc, queries, max)
DB->>AD: count(Document, queries, max)
AD->>AD: id = Document.getId()<br/>attrs = Document.getAttribute("attributes", [])
AD->>SQL: SELECT COUNT(*) FROM id WHERE getSQLConditions(queries, attrs)
SQL-->>AD: rows count
AD-->>DB: int
DB-->>Client: int
rect rgba(220,245,255,0.3)
note right of AD: New: pass collection attributes<br/>into condition builder
end
sequenceDiagram
autonumber
actor Client
participant DB as Database
participant AD as Adapter (MariaDB/Postgres)
participant SQL as SQL Engine
Client->>DB: sum(collectionDoc, attribute, queries, max)
DB->>AD: sum(Document, attribute, queries, max)
AD->>AD: id = Document.getId()<br/>attrs = Document.getAttribute("attributes", [])
AD->>SQL: SELECT SUM(attribute) FROM id WHERE getSQLConditions(queries, attrs)
SQL-->>AD: sum value
AD-->>DB: float|int
DB-->>Client: float|int
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
1923-1933: Critical: Spatial defaults always throw due to switch fall-through (missing break)In validateDefaultTypes(), the spatial cases (point/linestring/polygon) validate the default type but then fall through to the default branch, which throws “Unknown attribute type”. This makes it impossible to set valid defaults for spatial attributes.
Fix by adding a break after the spatial validation block.
Apply this diff:
case self::VAR_POINT: case self::VAR_LINESTRING: case self::VAR_POLYGON: // Spatial types expect arrays as default values if ($defaultType !== 'array') { throw new DatabaseException('Default value for spatial type ' . $type . ' must be an array'); } - // no break + break; default: throw new DatabaseException('Unknown attribute type: ' . $type . '. Must be one of ' . self::VAR_STRING . ', ' . self::VAR_INTEGER . ', ' . self::VAR_FLOAT . ', ' . self::VAR_BOOLEAN . ', ' . self::VAR_DATETIME . ', ' . self::VAR_RELATIONSHIP . ', ' . self::VAR_POINT . ', ' . self::VAR_LINESTRING . ', ' . self::VAR_POLYGON);
🧹 Nitpick comments (6)
src/Database/Adapter.php (2)
784-794: Clarify docblock to explain why a Document is now required (spatial-aware conditions)A one‑line note will make the intent obvious to maintainers and adapter authors.
/** * Sum an attribute * - * @param Document $collection + * Note: The full collection Document is required so adapters can resolve attribute metadata + * (including spatial attributes) when building SQL conditions. + * + * @param Document $collection * @param string $attribute * @param array<Query> $queries * @param int|null $max * * @return int|float */
795-806: Mirror the docblock clarification for count() as wellKeep the two aggregate methods consistent and self‑documenting.
/** * Count Documents * - * @param Document $collection + * Note: The full collection Document is required so adapters can resolve attribute metadata + * (including spatial attributes) when building SQL conditions. + * + * @param Document $collection * @param array<Query> $queries * @param int|null $max * * @return int */src/Database/Database.php (2)
6382-6390: Docblock nit: “Pass $max=0 for unlimited count” is misleading in sum()sum()’s PHPDoc says “Pass $max=0 for unlimited count”, but adapters treat non-null $max as a LIMIT, so 0 would yield LIMIT 0. Suggest clarifying that null means no limit and $max caps rows considered.
Proposed wording:
- * Sum an attribute for all the documents. Pass $max=0 for unlimited count + * Sum an attribute for all documents. + * $max limits the number of rows considered; pass null for no limit.
6416-6418: Optional: Filter-only queries before sum(), mirroring count()count() strips non-filter queries via Query::groupByType(...)[‘filters’] before convertQueries(). sum() forwards the full array. For symmetry and to avoid any non-filter noise reaching adapters, consider aligning sum().
Apply this diff:
- $queries = $this->convertQueries($collection, $queries); + $grouped = Query::groupByType($queries); + $queries = $this->convertQueries($collection, $grouped['filters']);src/Database/Adapter/MariaDB.php (2)
1551-1619: COUNT(): now correctly attribute-aware for spatial predicates — LGTM.
- Accepting
Document $collectionand passing$attributesintogetSQLConditions(..., attributes:$attributes)allowsgetSQLConditionto identify spatial types and generate ST_* SQL for filters used with COUNT.- Bind handling and LIMIT support are preserved.
Nit: variable naming across count/sum differs (
$attributesvs$collectionAttributes). Consider standardizing for readability.
1633-1695: SUM(): spatial-aware conditions wired correctly; quoting prevents SQL injection — LGTM.
- Passing collection attributes into
getSQLConditionsaligns SUM with FIND behavior for spatial filters.- Column name is safely quoted via
$this->quote($attribute).- Binds use
getPDOType, which mapsdoubletoPDO::PARAM_STR; consistent with existing code paths.Optional: consider validating that
$attributeis numeric per schema before building the SUM to fail-fast on misuse; not required for this PR.
📜 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 (6)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(4 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/Postgres.php(4 hunks)src/Database/Database.php(2 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/Database/Adapter.php (6)
src/Database/Adapter/MariaDB.php (2)
sum(1633-1695)count(1558-1620)src/Database/Adapter/Pool.php (2)
sum(268-271)count(273-276)src/Database/Adapter/Postgres.php (2)
sum(1720-1782)count(1644-1707)src/Database/Database.php (2)
sum(6394-6423)count(6343-6379)src/Database/Document.php (1)
Document(12-462)src/Database/Validator/Queries/Document.php (1)
Document(10-43)
src/Database/Adapter/MariaDB.php (6)
src/Database/Adapter.php (2)
count(804-804)sum(793-793)src/Database/Adapter/Pool.php (2)
count(273-276)sum(268-271)src/Database/Adapter/Postgres.php (2)
count(1644-1707)sum(1720-1782)src/Database/Database.php (2)
count(6343-6379)sum(6394-6423)src/Database/Document.php (3)
Document(12-462)getAttribute(216-223)getId(63-66)src/Database/Adapter/SQL.php (1)
getSQLConditions(1768-1785)
src/Database/Adapter/Pool.php (6)
src/Database/Adapter.php (2)
sum(793-793)count(804-804)src/Database/Adapter/MariaDB.php (2)
sum(1633-1695)count(1558-1620)src/Database/Adapter/Postgres.php (2)
sum(1720-1782)count(1644-1707)src/Database/Database.php (2)
sum(6394-6423)count(6343-6379)src/Database/Document.php (1)
Document(12-462)src/Database/Mirror.php (1)
delegate(88-103)
src/Database/Database.php (4)
src/Database/Adapter.php (2)
count(804-804)sum(793-793)src/Database/Adapter/MariaDB.php (2)
count(1558-1620)sum(1633-1695)src/Database/Adapter/Pool.php (2)
count(273-276)sum(268-271)src/Database/Adapter/Postgres.php (2)
count(1644-1707)sum(1720-1782)
tests/e2e/Adapter/Scopes/SpatialTests.php (7)
src/Database/Adapter.php (10)
getSupportForSpatialAttributes(1036-1036)createCollection(505-505)createAttribute(537-537)createIndex(649-649)createDocument(680-680)count(804-804)find(781-781)sum(793-793)getSupportForBoundaryInclusiveContains(1057-1057)deleteCollection(514-514)src/Database/Database.php (10)
getAdapter(1111-1114)createCollection(1240-1358)createAttribute(1588-1639)Database(37-7129)createIndex(3070-3222)createDocument(3666-3757)count(6343-6379)find(6127-6246)sum(6394-6423)deleteCollection(1519-1562)src/Database/Adapter/MariaDB.php (9)
getSupportForSpatialAttributes(2157-2160)createCollection(77-226)createIndex(716-779)createDocument(823-934)count(1558-1620)find(1374-1546)sum(1633-1695)getSupportForBoundaryInclusiveContains(2177-2180)deleteCollection(321-336)src/Database/Adapter/Postgres.php (10)
getSupportForSpatialAttributes(2261-2264)createCollection(192-336)createAttribute(447-466)createIndex(841-901)createDocument(960-1060)count(1644-1707)find(1464-1633)sum(1720-1782)getSupportForBoundaryInclusiveContains(2281-2284)deleteCollection(413-421)src/Database/Adapter/SQLite.php (6)
getSupportForSpatialAttributes(970-973)createCollection(141-227)createIndex(454-479)createDocument(521-629)getSupportForBoundaryInclusiveContains(1262-1265)deleteCollection(288-307)src/Database/Query.php (5)
Query(8-1041)distanceLessThan(941-944)distanceGreaterThan(929-932)contains(501-504)notContains(513-516)src/Database/Adapter/MySQL.php (1)
getSupportForBoundaryInclusiveContains(117-120)
src/Database/Adapter/Postgres.php (5)
src/Database/Adapter.php (2)
count(804-804)sum(793-793)src/Database/Adapter/MariaDB.php (2)
count(1558-1620)sum(1633-1695)src/Database/Database.php (2)
count(6343-6379)sum(6394-6423)src/Database/Document.php (3)
Document(12-462)getAttribute(216-223)getId(63-66)src/Database/Adapter/SQL.php (1)
getSQLConditions(1768-1785)
⏰ 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 (10)
src/Database/Adapter/Pool.php (2)
268-271: LGTM: sum() now accepts Document; delegation remains correctSignature matches the updated Adapter contract and safely forwards via func_get_args().
273-276: LGTM: count() signature updated to Document; consistent with adaptersConsistent with MariaDB/Postgres implementations and Database caller. No behavior change beyond type alignment.
src/Database/Database.php (3)
6373-6375: LGTM: count() now passes the collection Document to the adapterThis aligns with the updated Adapter signature and enables spatial-aware condition building downstream. No functional concerns here.
6418-6419: LGTM: sum() now passes the collection Document to the adapterMatches the interface change and keeps behavior consistent with find(), which already forwards the Document.
6394-6423: Confirm authorization parity: sum() vs count()count() conditionally wraps the adapter call in Authorization::skip() when collection-level READ is granted. sum() doesn’t, relying solely on adapter-side permission conditions. If this difference is intentional, all good; if not, mirror count()’s pattern.
If you want parity, here’s the minimal change:
- $sum = $this->adapter->sum($collection, $attribute, $queries, $max); + $getSum = fn () => $this->adapter->sum($collection, $attribute, $queries, $max); + $authorization = new Authorization(self::PERMISSION_READ); + $skipAuth = $authorization->isValid($collection->getRead()); + $sum = $skipAuth ? Authorization::skip($getSum) : $getSum();tests/e2e/Adapter/Scopes/SpatialTests.php (2)
1649-1734: Good coverage: spatial filters now influence count and sum as expected.The test exercises distanceLessThan/GreaterThan and contains/notContains against COUNT and SUM, validating the adapter changes end-to-end. Assertions and dataset look sound.
1649-1734: Incorrect duplicate-removal suggestion—only the typo needs fixing
The methodtestSptialAggregationis defined only once intests/e2e/Adapter/Scopes/SpatialTests.php, so there is no duplicate to remove. Please apply just the typo fix:--- a/tests/e2e/Adapter/Scopes/SpatialTests.php +++ b/tests/e2e/Adapter/Scopes/SpatialTests.php @@ -1649,7 +1649,7 @@ - public function testSptialAggregation(): void + public function testSpatialAggregation(): voidLikely an incorrect or invalid review comment.
src/Database/Adapter/Postgres.php (3)
1636-1707: COUNT(): attribute-aware spatial condition building — LGTM.
- Moving to
Document $collectionand threading$collectionAttributesintogetSQLConditionsenables ST_* predicates for COUNT.- Uses
$this->execute($stmt)so statement_timeout still applies; good.
1712-1782: SUM(): spatial-aware, boundary semantics retained — LGTM.
- Attribute list is passed into
getSQLConditions, aligning with FIND behavior.- Attribute quoting looks correct for Postgres (
"), and execution path keeps timeouts intact.- With
handleSpatialQueries()usingST_Coversfor contains, boundary-inclusive tests remain consistent with adapters reporting support.
1636-1645: Adapter method signatures are consistent across all adapters
- Postgres (
src/Database/Adapter/Postgres.php):
- count(Document $collection, …) at line 1644
- sum(Document $collection, …) at line 1720
- MariaDB (
src/Database/Adapter/MariaDB.php):
- count(Document $collection, …) at line 1558
- sum(Document $collection, …) at line 1633
- Pool (
src/Database/Adapter/Pool.php):
- sum(Document $collection, …) at line 268
- count(Document $collection, …) at line 273
- MySQL and SQLite inherit from MariaDB and thus use the correct signatures.
No occurrences of methods still declaring
string $collectionwere found. All adapters now implement the updated abstract interface.
| * @return int|float | ||
| */ | ||
| abstract public function sum(string $collection, string $attribute, array $queries = [], ?int $max = null): float|int; | ||
| abstract public function sum(Document $collection, string $attribute, array $queries = [], ?int $max = null): float|int; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Abstract signature change to Document is a breaking change; verify all adapters and docs are updated
Switching count()/sum() to accept Document instead of string will break any third‑party adapters extending Adapter that haven’t adopted the new signature. The core MariaDB/Postgres/Pool classes in this PR appear aligned, but please verify all in‑tree adapters and update the upgrade guide/changelog to flag this BC break.
Run this script to spot any lingering old signatures:
Also ensure release notes explicitly call out this change and provide a short migration snippet for community adapters.
Also applies to: 804-804
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Looking for old signatures (string \$collection) in adapter implementations..."
rg -nP --type-add 'php:*.php' --type php -C1 $'class\\s+\\w+\\s+extends\\s+\\\\?Utopia\\\\Database\\\\Adapter|class\\s+\\w+\\s+extends\\s+Adapter' | cat >/dev/null # touch adapters for context
echo "Old sum signatures:"
rg -nP --type php $'function\\s+sum\\s*\\(\\s*string\\s+\\$collection'
echo "Old count signatures:"
rg -nP --type php $'function\\s+count\\s*\\(\\s*string\\s+\\$collection\\b'
echo "New sum signatures (Document \$collection):"
rg -nP --type php $'function\\s+sum\\s*\\(\\s*\\\\?Utopia\\\\Database\\\\Document\\s+\\$collection|function\\s+sum\\s*\\(\\s*Document\\s+\\$collection'
echo "New count signatures (Document \$collection):"
rg -nP --type php $'function\\s+count\\s*\\(\\s*\\\\?Utopia\\\\Database\\\\Document\\s+\\$collection|function\\s+count\\s*\\(\\s*Document\\s+\\$collection'Length of output: 2423
🔄 Confirmed adapter signatures updated—please update upgrade guide & CHANGELOG for BC break
All in-tree adapters have been aligned to the new Document $collection signature for both sum() and count():
- Pool, Postgres, and MariaDB adapters no longer use
string $collectionin theirsum()/count()methods. - No other adapter implementations with the old signature were found.
Next steps:
- Update your upgrade guide (e.g. docs/upgrading.md or UPGRADE.md) to call out the breaking change in
sum()andcount()signatures. - Add an entry to CHANGELOG.md detailing:
- The signature change from
string $collectiontoDocument $collection. - A migration snippet for community adapters, for example:
- public function sum(string $collection, string $attribute, array $queries = [], ?int $max = null): float|int + use Utopia\Database\Document; + + public function sum(Document $collection, string $attribute, array $queries = [], ?int $max = null): float|int
- The signature change from
- Ensure the guide notes that third-party adapters must import
Utopia\Database\Documentand update their method signatures accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Database/Adapter.php around line 793, the public signatures for sum() and
count() were changed to accept Document $collection instead of string
$collection; update the project documentation and changelog to reflect this
breaking change by: adding an entry in CHANGELOG.md that states the signature
change from string $collection to Document $collection and includes a short
migration snippet showing replacing the string parameter with a
Utopia\Database\Document-typed parameter; updating the upgrade guide
(docs/upgrading.md or UPGRADE.md) with a clear note for maintainers of
third‑party adapters instructing them to import Utopia\Database\Document and
update their sum()/count() method signatures accordingly; and ensure the guide
mentions both in-tree adapters updated (Pool, Postgres, MariaDB) as examples and
the exact namespace to import.
| public function count(Document $collection, array $queries = [], ?int $max = null): int | ||
| { | ||
| $attributes = $collection->getAttribute("attributes", []); | ||
| $collection = $collection->getId(); | ||
| $name = $this->filter($collection); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm attribute shape consistency passed into getSQLConditions.
In some MariaDB paths (e.g., createIndex) attributes are JSON-decoded; here they’re taken directly from $collection->getAttribute('attributes', []). Ensure $attributes is the same structure getAttributeType() expects (array of attribute definitions) for all callers of count/sum.
You can sanity-check with:
Also applies to: 1634-1637
🏁 Script executed:
#!/bin/bash
# Inspect how attributes are shaped where getSQLConditions is called
rg -nP --type=php -C2 'getSQLConditions\(\$queries,\s*\$binds,.*attributes:'Length of output: 2540
Here is the additional verification script to inspect the implementation of createIndex and any JSON decoding of attributes in the MariaDB adapter:
🏁 Script executed:
#!/bin/bash
# Locate createIndex implementation in MariaDB adapter
rg -nP --type=php 'function createIndex' -C10 src/Database/Adapter/MariaDB.php
# Check for JSON decoding in MariaDB adapter
rg -nP 'json_decode' -C5 src/Database/Adapter/MariaDB.phpLength of output: 2029
🏁 Script executed:
#!/bin/bash
# Find the getSQLConditions implementation in the MariaDB adapter
rg -nP --type=php 'function getSQLConditions' -C5 src/Database/Adapter/MariaDB.phpLength of output: 84
🏁 Script executed:
#!/bin/bash
# Locate the definition of getSQLConditions across the repository
rg -nR "function getSQLConditions" -C5 -n .
# Locate the definition of getAttributeType across the repository
rg -nR "function getAttributeType" -C5 -n .
# Find the Document class declaration to inspect getAttribute
rg -nR "class\s\+Document" -C5 -n .
# Inspect Document::getAttribute implementation
rg -nR "function\s\+getAttribute" -C5 -n srcLength of output: 1796
🏁 Script executed:
#!/bin/bash
# Locate getSQLConditions implementation
rg -nP 'function\s+getSQLConditions' -C5 -n .
# Locate getAttributeType implementation
rg -nP 'function\s+getAttributeType' -C5 -n .
# Find the Document class file
DOCUMENT_PATH=$(fd -e php Document.php | head -n1)
echo "Document file: $DOCUMENT_PATH"
# Inspect Document::getAttribute in that file
rg -nP 'function\s+getAttribute' -C3 -n "$DOCUMENT_PATH"Length of output: 2320
Ensure JSON-decoding of attributes in count() and sum()
Both count() (around line 1558) and sum() (around line 1634) in
src/Database/Adapter/MariaDB.php pull
$attributes = $collection->getAttribute('attributes', []);directly from the Document, which yields the raw JSON string stored in the DB. Meanwhile, createIndex() explicitly does:
$collectionAttributes = json_decode($collection->getAttribute('attributes', []), true);and getSQLConditions(array $queries, array &$binds, string $sep, array $attributes) / getAttributeType() expect an array of attribute-definition arrays, not a JSON string.
• src/Database/Adapter/MariaDB.php:1558-1562 (count())
• src/Database/Adapter/MariaDB.php:1634-1637 (sum())
To fix, mirror createIndex()’s approach in both count() and sum():
- $attributes = $collection->getAttribute('attributes', []);
+ $attributes = json_decode($collection->getAttribute('attributes', '[]'), true);This ensures $attributes is always an array of definitions before it’s passed to getSQLConditions().
🤖 Prompt for AI Agents
In src/Database/Adapter/MariaDB.php around lines 1558-1562 and 1634-1637, the
code grabs $attributes directly from $collection->getAttribute('attributes', [])
which returns the raw JSON string from the DB; change both occurrences to
json_decode($collection->getAttribute('attributes', []), true) (with a true
second arg and default to an empty array) so $attributes is always an
associative array of attribute definitions before being passed to
getSQLConditions()/getAttributeType().
|
|
||
| // COUNT with spatial distance filter | ||
| $queries = [ | ||
| Query::distanceLessThan('loc', [[[10.0, 10.0], 0.1]]) |
There was a problem hiding this comment.
Looking at this here, let's make this two parameters in the query class, we can merge into values internally, but from outside would be much cleaner to have Query::distanceLessThan('loc', geometry: [10, 10], distance: 0.1), same for all the distance queries. While we're at it, let's rename distance to distanceEqual and distanceNotEqual
2 participants
Summary by CodeRabbit