meters param for distance queries, srid postgres update and fixed structure exception not getting raised for spatial types #682
Conversation
WalkthroughAdds SRID constant, enforces spatial validation in Database::encodeSpatialData (throws StructureException on invalid), refactors Spatial validator to instance-based validators with detailed messages, adds meters flag for distance queries, centralizes distance SQL in adapters, and introduces unit and e2e tests for spatial validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant DB as Database::encodeSpatialData
participant V as Spatial (instance validator)
participant Adapter as DB Adapter (MySQL/Postgres/MariaDB)
Client->>DB: send spatial value + type
DB->>V: isValid(value)
alt valid
V-->>DB: true
DB->>Adapter: build spatial SQL / bind params (may call handleDistanceSpatialQueries)
Adapter-->>DB: SQL fragment
DB-->>Client: encoded spatial payload
else invalid
V-->>DB: false (+ getDescription())
DB-->>Client: throw StructureException(getDescription)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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: 3
🧹 Nitpick comments (5)
src/Database/Validator/Spatial.php (2)
112-115: Avoid dangling colon when no message is set.- return 'Value must be a valid ' . $this->spatialType . ": {$this->message}"; + return 'Value must be a valid ' . $this->spatialType . ($this->message ? ": {$this->message}" : '');
81-98: Optionally strengthen polygon validation (min 4 points and closed rings).Helps prevent malformed polygons at source.
foreach ($value as $ring) { if (!is_array($ring) || empty($ring)) { $this->message = 'Each ring in Polygon must be an array of points'; return false; } foreach ($ring as $point) { if (!is_array($point) || count($point) !== 2) { $this->message = 'Each point in Polygon ring must be an array of two values [x, y]'; return false; } if (!is_numeric($point[0]) || !is_numeric($point[1])) { $this->message = 'Each point in Polygon ring must have numeric coordinates'; return false; } } + if (\count($ring) < 4) { + $this->message = 'Each polygon ring must have at least four points (first and last identical).'; + return false; + } + $first = $ring[0]; + $last = $ring[\count($ring) - 1]; + if ($first[0] != $last[0] || $first[1] != $last[1]) { + $this->message = 'Polygon rings must be closed (first and last points equal).'; + return false; + } }src/Database/Database.php (1)
7136-7138: Should spatial validation honor skipValidation()?encodeSpatialData now always validates and throws, even when validation is disabled via skipValidation/disableValidation. Confirm desired behavior; if not, gate on $this->validate.
- if (!$validator->isValid($value)) { + if ($this->validate && !$validator->isValid($value)) { throw new StructureException($validator->getDescription()); }Note: If you gate it, consider that invalid arrays may still cause downstream notices during WKT assembly; add minimal guards if needed.
tests/e2e/Adapter/Scopes/SpatialTests.php (2)
1556-1568: Harden the negative update test to be future-proofPass a no-op query so the test won’t accidentally update all documents if validation order changes, and make the failure message clearer.
- try { - $database->updateDocuments($collectionName, new Document([ - 'name' => 'Updated Location', - 'location' => [15.0, 25.0], - 'area' => [15.0, 25.0] // invalid polygon - ])); - $this->fail("fail to throw structure exception for the invalid spatial type"); + try { + $database->updateDocuments( + $collectionName, + new Document([ + 'name' => 'Updated Location', + 'location' => [15.0, 25.0], + 'area' => [15.0, 25.0] // invalid polygon + ]), + [Query::equal('$id', ['__never__'])] + ); + $this->fail('Expected StructureException for invalid polygon update');
1986-2070: Guarantee cleanup with try/finally to avoid state leakage across testsIf an assertion throws before the explicit delete, the collection remains. Wrap the body in try/finally.
- public function testInvalidSpatialTypes(): void - { + public function testInvalidSpatialTypes(): void + { /** @var Database $database */ $database = static::getDatabase(); if (!$database->getAdapter()->getSupportForSpatialAttributes()) { $this->markTestSkipped('Adapter does not support spatial attributes'); } $collectionName = 'test_invalid_spatial_types'; - - $attributes = [ + try { + $attributes = [ new Document([ '$id' => ID::custom('pointAttr'), 'type' => Database::VAR_POINT, 'size' => 0, 'required' => false, 'signed' => true, 'array' => false, 'filters' => [], ]), new Document([ '$id' => ID::custom('lineAttr'), 'type' => Database::VAR_LINESTRING, 'size' => 0, 'required' => false, 'signed' => true, 'array' => false, 'filters' => [], ]), new Document([ '$id' => ID::custom('polyAttr'), 'type' => Database::VAR_POLYGON, 'size' => 0, 'required' => false, 'signed' => true, 'array' => false, 'filters' => [], ]) - ]; - - $database->createCollection($collectionName, $attributes); + ]; + + $database->createCollection($collectionName, $attributes); // ❌ Invalid Point (must be [x, y]) try { $database->createDocument($collectionName, new Document([ 'pointAttr' => [10.0], // only 1 coordinate ])); $this->fail("Expected StructureException for invalid point"); } catch (\Throwable $th) { $this->assertInstanceOf(StructureException::class, $th); } // ❌ Invalid LineString (must be [[x,y],[x,y],...], at least 2 points) try { $database->createDocument($collectionName, new Document([ 'lineAttr' => [[10.0, 20.0]], // only one point ])); $this->fail("Expected StructureException for invalid line"); } catch (\Throwable $th) { $this->assertInstanceOf(StructureException::class, $th); } try { $database->createDocument($collectionName, new Document([ 'lineAttr' => [10.0, 20.0], // not an array of arrays ])); $this->fail("Expected StructureException for invalid line structure"); } catch (\Throwable $th) { $this->assertInstanceOf(StructureException::class, $th); } try { $database->createDocument($collectionName, new Document([ 'polyAttr' => [10.0, 20.0] // not an array of arrays ])); $this->fail("Expected StructureException for invalid polygon structure"); } catch (\Throwable $th) { $this->assertInstanceOf(StructureException::class, $th); } - - // Cleanup - $database->deleteCollection($collectionName); + } finally { + $database->deleteCollection($collectionName); + } }
📜 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/Database.php(1 hunks)src/Database/Validator/Spatial.php(4 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Database/Database.php (1)
src/Database/Validator/Spatial.php (2)
isValid(130-159)getDescription(112-115)
tests/e2e/Adapter/Scopes/SpatialTests.php (2)
src/Database/Database.php (2)
Database(37-7228)updateDocuments(4482-4672)src/Database/Validator/Structure.php (1)
Structure(18-431)
src/Database/Validator/Spatial.php (1)
src/Database/Database.php (1)
Database(37-7228)
🪛 GitHub Actions: CodeQL
src/Database/Validator/Spatial.php
[error] 21-21: PHPStan: Method Utopia\Database\Validator\Spatial::validatePoint() has parameter $value with no value type specified in iterable type array.
[error] 39-39: PHPStan: Method Utopia\Database\Validator\Spatial::validateLineString() has parameter $value with no value type specified in iterable type array.
[error] 64-64: PHPStan: Method Utopia\Database\Validator\Spatial::validatePolygon() has parameter $value with no value type specified in iterable type array.
⏰ 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/Database.php (1)
7136-7138: LGTM: now throws StructureException on invalid spatial arrays.This addresses the original bug by surfacing validator failures.
tests/e2e/Adapter/Scopes/SpatialTests.php (1)
8-8: Correct exception alias importThe alias is appropriate and used in the new negative tests.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Database/Validator/Spatial.php (2)
21-25: Use precise docblocks (tuple/list shapes) to satisfy PHPStan/Psalm.Current annotations are either incorrect (
array<int|float>) or too loose (array<mixed>). Use exact shapes for coordinates, lines, and polygons. This also addresses prior feedback on the same lines.- /** - * @param array<int|float> $value - * @return bool - */ + /** + * @param array{0:int|float,1:int|float} $value + * @return bool + */ protected function validatePoint(array $value): bool- /** - * @param array<mixed> $value - * @return bool - */ + /** + * @param list<array{0:int|float,1:int|float}> $value + * @return bool + */ protected function validateLineString(array $value): bool- /** - * @param array<mixed> $value - * @return bool - */ + /** + * Accepts either a single ring (list of points) or multiple rings (list of list of points). + * + * @param list<array{0:int|float,1:int|float}>|list<list<array{0:int|float,1:int|float}>> $value + * @return bool + */ protected function validatePolygon(array $value): boolAlso applies to: 37-41, 65-69
140-142: Set message and enforce WKT geometry type to match the attribute.Prevents invalid WKT returning empty descriptions and stops mismatched types (e.g., LINESTRING for point) from passing.
- if (is_string($value)) { - return self::isWKTString($value); - } + if (is_string($value)) { + $wkt = trim($value); + if (!self::isWKTString($wkt)) { + $this->message = 'Invalid WKT string'; + return false; + } + $expected = strtoupper($this->spatialType); + if (!preg_match('/^' . preg_quote($expected, '/') . '\s*\(/', strtoupper($wkt))) { + $this->message = "WKT type does not match expected {$this->spatialType}"; + return false; + } + return true; + }
🧹 Nitpick comments (3)
src/Database/Validator/Spatial.php (3)
116-119: Avoid dangling colon when no message is set.- return 'Value must be a valid ' . $this->spatialType . ": {$this->message}"; + return $this->message !== '' + ? 'Value must be a valid ' . $this->spatialType . ': ' . $this->message + : 'Value must be a valid ' . $this->spatialType . '.';
85-102: Polygon rings: require a minimum number of points.Add a simple cardinality check to catch degenerate rings early. If closure is required, we can extend this later.
foreach ($value as $ring) { if (!is_array($ring) || empty($ring)) { $this->message = 'Each ring in Polygon must be an array of points'; return false; } + if (count($ring) < 3) { + $this->message = 'Each ring in Polygon must contain at least three points'; + return false; + } + foreach ($ring as $point) { if (!is_array($point) || count($point) !== 2) { $this->message = 'Each point in Polygon ring must be an array of two values [x, y]'; return false; }
110-114: Optional: tighten WKT detection to require a trailing ')'.Keeps it lightweight while avoiding obvious malformed inputs like 'POINT('.
- return (bool) preg_match('/^(POINT|LINESTRING|POLYGON)\s*\(/i', $value); + return (bool) preg_match('/^(?:POINT\s*\([^()]+\)|LINESTRING\s*\([^()]+\)|POLYGON\s*\(.+\))$/i', $value);
📜 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 (1)
src/Database/Validator/Spatial.php(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Validator/Spatial.php (1)
src/Database/Database.php (2)
count(6404-6440)Database(37-7228)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (1)
src/Database/Validator/Spatial.php (1)
136-139: Nulls can’t reach encodeSpatialData thanks to upstream is_array guard
The call at Database.php line 6556 wrapsencodeSpatialDatainif (is_array($node)), sonullvalues never flow into it.
ArnabChatterjee20k
changed the title
| class Spatial extends Validator | ||
| { | ||
| private string $spatialType; | ||
| protected string $message = ''; |
There was a problem hiding this comment.
I think we can use $description from the base class instead
There was a problem hiding this comment.
there is no $description var in the validator , just a getDescription which gives the error message
other validator using the message only
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 (3)
src/Database/Validator/Query/Filter.php (1)
266-271: Avoid breaking change; accept both 2- and 3-tuple distance params and fix error text.Current check rejects legacy 2-tuple [geometry, distance]. Allow 2 or 3 (when meters is provided) and update the message.
- if (count($value->getValues()) !== 1 || !is_array($value->getValues()[0]) || count($value->getValues()[0]) !== 3) { - $this->message = 'Distance query requires [[geometry, distance]] parameters'; + if ( + count($value->getValues()) !== 1 || + !is_array($value->getValues()[0]) || + !in_array(count($value->getValues()[0]), [2, 3], true) + ) { + $this->message = 'Distance query requires [[geometry, distance]] or [[geometry, distance, meters]] parameters'; return false; }Optional: also validate types to fail fast if provided.
} - return $this->isValidAttributeAndValues($attribute, $value->getValues(), $method); + $params = $value->getValues()[0]; + if (!is_array($params[0]) || !is_numeric($params[1]) || (isset($params[2]) && !is_bool($params[2]))) { + $this->message = 'Distance query expects geometry array, numeric distance, and optional boolean meters flag'; + return false; + } + return $this->isValidAttributeAndValues($attribute, $value->getValues(), $method);src/Database/Adapter/Postgres.php (1)
1516-1521: Add SRID to ST_GeomFromText for CROSSES/NOT CROSSESMixed/unknown SRIDs can error in PostGIS. Others in this method already specify SRID.
- return "ST_Crosses({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0))"; + return "ST_Crosses({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . "))"; ... - return "NOT ST_Crosses({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0))"; + return "NOT ST_Crosses({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . "))";tests/e2e/Adapter/Scopes/SpatialTests.php (1)
215-218: Ensure test cleanup runs.The collection is not deleted, which can leak state and cause flakiness across runs.
- } finally { - // $database->deleteCollection($collectionName); - } + } finally { + $database->deleteCollection($collectionName); + }
♻️ Duplicate comments (5)
src/Database/Validator/Spatial.php (4)
21-25: Add precise param shapes for PHPStan and self-documentationTighten iterable types for validators as previously suggested.
- /** - * @param array<mixed> $value - * @return bool - */ + /** + * @param array{0:int|float,1:int|float} $value + * @return bool + */ protected function validatePoint(array $value): bool
42-46: Annotate LineString point list shape- /** - * @param array<mixed> $value - * @return bool - */ + /** + * @param list<array{0:int|float,1:int|float}> $value + * @return bool + */ protected function validateLineString(array $value): bool
70-74: Annotate Polygon rings/points shape (single or multi-ring)- /** - * @param array<mixed> $value - * @return bool - */ + /** + * Accepts either a single ring (list of points) or multiple rings (list of list of points). + * + * @param list<array{0:int|float,1:int|float}>|list<list<array{0:int|float,1:int|float}>> $value + * @return bool + */ protected function validatePolygon(array $value): bool
156-159: Set message and enforce WKT type to match expected geometryProvide actionable error text and prevent mismatched WKT types.
- if (is_string($value)) { - return self::isWKTString($value); - } + if (is_string($value)) { + $wkt = trim($value); + if (!self::isWKTString($wkt)) { + $this->message = 'Invalid WKT string'; + return false; + } + $expected = strtoupper($this->spatialType); + $type = strtoupper(strtok($wkt, '(')); + if ($type !== $expected) { + $this->message = "WKT type does not match expected {$this->spatialType}"; + return false; + } + return true; + }src/Database/Database.php (1)
6553-6561: Prevent WKT strings with the wrong geometry from bypassing validationCurrently only arrays are validated/encoded; WKT strings pass through without type-check, so a LINESTRING WKT can land in a POINT attribute. Validate strings too (or enforce type-specific WKT) before filters. Example patch:
if (in_array($attributeType, Database::SPATIAL_TYPES)) { - if (is_array($node)) { - $node = $this->encodeSpatialData($node, $attributeType); - } + if (is_array($node)) { + $node = $this->encodeSpatialData($node, $attributeType); + } elseif (is_string($node)) { + // Validate WKT (and geometry kind) when user supplies a string + $validator = new Spatial($attributeType); + if ($this->validate && !$validator->isValid($node)) { + throw new StructureException($validator->getDescription()); + } + $upper = strtoupper(ltrim($node)); + $expected = match ($attributeType) { + self::VAR_POINT => 'POINT(', + self::VAR_LINESTRING => 'LINESTRING(', + self::VAR_POLYGON => 'POLYGON(', + default => '', + }; + if ($expected && !str_starts_with($upper, $expected)) { + throw new StructureException('Value must be a valid ' . $attributeType . ' WKT.'); + } + } }
🧹 Nitpick comments (15)
src/Database/Query.php (1)
906-912: Minor: spacing in constructed values array.Tight array commas hurt readability; match codebase style.
- return new self(self::TYPE_DISTANCE_EQUAL, $attribute, [[$values,$distance,$meters]]); + return new self(self::TYPE_DISTANCE_EQUAL, $attribute, [[$values, $distance, $meters]]);- return new self(self::TYPE_DISTANCE_LESS_THAN, $attribute, [[$values,$distance,$meters]]); + return new self(self::TYPE_DISTANCE_LESS_THAN, $attribute, [[$values, $distance, $meters]]);Also applies to: 951-954
src/Database/Adapter/MariaDB.php (1)
1365-1394: Optional: extract Earth radius to a shared constant.Use a single source of truth (e.g., Database::EARTH_RADIUS_M) to keep MariaDB/MySQL consistent.
- if ($meters) { - return "ST_DISTANCE_SPHERE({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0), 6371000) {$operator} :{$placeholder}_1"; - } + if ($meters) { + $radius = \Utopia\Database\Database::EARTH_RADIUS_M ?? 6371000; + return "ST_DISTANCE_SPHERE({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0), {$radius}) {$operator} :{$placeholder}_1"; + }src/Database/Validator/Spatial.php (3)
24-37: Point: ensure indexed keys [0,1] exist for clearer failuresPrevents associative arrays from failing with a vague numeric message.
protected function validatePoint(array $value): bool { if (count($value) !== 2) { $this->message = 'Point must be an array of two numeric values [x, y]'; return false; } - - if (!is_numeric($value[0]) || !is_numeric($value[1])) { + if (!array_key_exists(0, $value) || !array_key_exists(1, $value)) { + $this->message = 'Point must use indexed keys [0, 1]'; + return false; + } + if (!is_numeric($value[0]) || !is_numeric($value[1])) { $this->message = 'Point coordinates must be numeric values'; return false; }
113-117: Polygon: loosen closure check to value-equalityStrict identity can fail on numeric type differences (e.g., 1 vs 1.0).
- if ($ring[0] !== $ring[count($ring) - 1]) { + if ($ring[0] != $ring[count($ring) - 1]) { $this->message = "Ring #{$ringIndex} must be closed (first point must equal last point)"; return false; }
132-135: Avoid trailing colon when no messageSmall UX polish for error text.
- return 'Value must be a valid ' . $this->spatialType . ": {$this->message}"; + return trim('Value must be a valid ' . $this->spatialType . ($this->message !== '' ? ": {$this->message}" : ''));src/Database/Database.php (3)
52-54: SRID constant: confirm adapter usage and clarify coordinate orderGood addition. Please verify all adapters (especially Postgres) source SRID from Database::SRID when constructing geometries/geographies so distance-in-meters queries are consistent. Also, consider noting in the comment that EPSG:4326 expects [lon, lat] (x, y).
Would you like a follow-up PR to document the lon/lat order explicitly in validator docs?
7188-7193: Support EWKT (SRID=xxxx;WKT) in decoder for PostgresIf adapters start returning EWKT, decodeSpatialData will miss it. Strip the SRID prefix first:
- $upper = strtoupper($wkt); + $wkt = trim($wkt); + if (preg_match('/^SRID=\d+;(.+)$/i', $wkt, $m)) { + $wkt = $m[1]; + } + $upper = strtoupper($wkt);
7213-7215: Fix WKT comment exampleUse space-separated coordinates in WKT.
- // POLYGON((x1,y1),(x2,y2)) + // POLYGON((x1 y1, x2 y2), ...)tests/e2e/Adapter/Scopes/SpatialTests.php (6)
21-23: Remove stray semicolon after if-block.Minor style nit; the trailing semicolon after the closing brace serves no purpose.
- if (!$database->getAdapter()->getSupportForSpatialAttributes()) { - $this->markTestSkipped('Adapter does not support spatial attributes'); - }; + if (!$database->getAdapter()->getSupportForSpatialAttributes()) { + $this->markTestSkipped('Adapter does not support spatial attributes'); + }
152-155: Use correct class casing for Query.Static calls should consistently use Query::..., not query::.... Functionally it works in PHP, but it’s inconsistent and hurts readability.
- 'equals' => query::equal('lineAttr', [[[1.0, 2.0], [3.0, 4.0]]]), // Exact same linestring - 'notEquals' => query::notEqual('lineAttr', [[[5.0, 6.0], [7.0, 8.0]]]), // Different linestring + 'equals' => Query::equal('lineAttr', [[[1.0, 2.0], [3.0, 4.0]]]), // Exact same linestring + 'notEquals' => Query::notEqual('lineAttr', [[[5.0, 6.0], [7.0, 8.0]]]), // Different linestring- 'equals' => query::equal('polyAttr', [[[[0.0, 0.0], [0.0, 10.0], [10.0, 10.0], [0.0, 0.0]]]]), // Exact same polygon - 'notEquals' => query::notEqual('polyAttr', [[[[20.0, 20.0], [20.0, 30.0], [30.0, 30.0], [20.0, 20.0]]]]), // Different polygon + 'equals' => Query::equal('polyAttr', [[[[0.0, 0.0], [0.0, 10.0], [10.0, 10.0], [0.0, 0.0]]]]), // Exact same polygon + 'notEquals' => Query::notEqual('polyAttr', [[[[20.0, 20.0], [20.0, 30.0], [30.0, 30.0], [20.0, 20.0]]]]), // Different polygon- query::notEqual('square', [[[0, 0], [0, 10], [10, 10], [10, 0], [0, 0]]]) // Different square + Query::notEqual('square', [[[0, 0], [0, 10], [10, 10], [10, 0], [0, 0]]]) // Different squareAlso applies to: 187-189, 1053-1053
163-165: Fix assertion messages to reference the correct attribute.These messages say polyAttr while the loop is for lineAttr. This confuses failures.
- $this->assertNotEmpty($result, sprintf('Failed spatial query: %s on polyAttr', $queryType)); - $this->assertEquals('doc1', $result[0]->getId(), sprintf('Incorrect document returned for %s on polyAttr', $queryType)); + $this->assertNotEmpty($result, sprintf('Failed spatial query: %s on lineAttr', $queryType)); + $this->assertEquals('doc1', $result[0]->getId(), sprintf('Incorrect document returned for %s on lineAttr', $queryType));
1383-1392: Don’t assert implicit ordering by distance without an explicit order.Unless the adapter guarantees distance ordering for distanceLessThan, asserting the first result is closest is brittle. Either add an explicit order-by-distance (if supported) or relax the assertion to set membership.
- $this->assertNotEmpty($orderedByDistance); - // First result should be closest to the reference point - $this->assertEquals('park1', $orderedByDistance[0]->getId()); + $this->assertNotEmpty($orderedByDistance); + $this->assertContains('park1', array_map(fn($d) => $d->getId(), $orderedByDistance));
1710-1710: Rename misspelled test method.Typo in method name (Sptial). Keeps naming consistent and discoverable.
- public function testSptialAggregation(): void + public function testSpatialAggregation(): void
1194-1207: Align comments with called operators.Comments say “distanceEqual” but the code uses distanceLessThan; fix to avoid confusion when reading failures.
- // Test distanceEqual queries between shapes + // Test distanceLessThan queries between shapes ... - // Test distanceEqual queries to find nearby shapes + // Test distanceLessThan queries to find nearby shapesAlso applies to: 1208-1221
tests/unit/Validator/SpatialTest.php (1)
75-84: Consider adding isValid() tests for WKT strings and nulls.You already test Spatial::isWKTString; also verify main isValid accepts WKT and nulls per contract.
Example additions (new test methods):
public function testWKTAcceptedByIsValid(): void { $p = new Spatial(Database::VAR_POINT); $l = new Spatial(Database::VAR_LINESTRING); $g = new Spatial(Database::VAR_POLYGON); $this->assertTrue($p->isValid('POINT(1 2)')); $this->assertTrue($l->isValid('LINESTRING(0 0,1 1)')); $this->assertTrue($g->isValid('POLYGON((0 0,1 0,1 1,0 1,0 0))')); } public function testNullIsValid(): void { $p = new Spatial(Database::VAR_POINT); $this->assertTrue($p->isValid(null)); }
📜 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 (9)
src/Database/Adapter/MariaDB.php(2 hunks)src/Database/Adapter/MySQL.php(2 hunks)src/Database/Adapter/Postgres.php(4 hunks)src/Database/Database.php(2 hunks)src/Database/Query.php(4 hunks)src/Database/Validator/Query/Filter.php(1 hunks)src/Database/Validator/Spatial.php(4 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(3 hunks)tests/unit/Validator/SpatialTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-14T06:35:30.429Z
Learnt from: ArnabChatterjee20k
PR: utopia-php/database#661
File: tests/e2e/Adapter/Scopes/SpatialTests.php:180-186
Timestamp: 2025-08-14T06:35:30.429Z
Learning: Query::distance method in Utopia\Database\Query expects an array of values parameter, where each value is a [geometry, distance] pair. So the correct format is Query::distance('attribute', [[[lat, lng], distance]]) where the outer array contains the values and each value is [geometry, distance].
Applied to files:
src/Database/Validator/Query/Filter.phpsrc/Database/Query.php
🧬 Code graph analysis (8)
tests/unit/Validator/SpatialTest.php (2)
src/Database/Database.php (1)
Database(37-7231)src/Database/Validator/Spatial.php (3)
Spatial(8-180)isValid(150-179)isWKTString(126-130)
src/Database/Validator/Query/Filter.php (2)
src/Database/Database.php (1)
count(6407-6443)src/Database/Query.php (1)
getValues(164-167)
src/Database/Database.php (1)
src/Database/Validator/Spatial.php (2)
isValid(150-179)getDescription(132-135)
src/Database/Adapter/MariaDB.php (4)
src/Database/Adapter/MySQL.php (1)
handleDistanceSpatialQueries(92-120)src/Database/Adapter/Postgres.php (1)
handleDistanceSpatialQueries(1465-1499)src/Database/Query.php (3)
Query(8-1051)getValues(164-167)getMethod(148-151)src/Database/Adapter/SQL.php (1)
convertArrayToWKT(2259-2298)
src/Database/Adapter/MySQL.php (4)
src/Database/Query.php (3)
Query(8-1051)getValues(164-167)getMethod(148-151)src/Database/Adapter/Postgres.php (1)
handleDistanceSpatialQueries(1465-1499)src/Database/Adapter/MariaDB.php (1)
handleDistanceSpatialQueries(1365-1394)src/Database/Adapter/SQL.php (1)
convertArrayToWKT(2259-2298)
tests/e2e/Adapter/Scopes/SpatialTests.php (2)
src/Database/Database.php (3)
Database(37-7231)updateDocuments(4485-4675)createDocument(3730-3821)src/Database/Validator/Structure.php (1)
Structure(18-431)
src/Database/Validator/Spatial.php (1)
src/Database/Database.php (2)
count(6407-6443)Database(37-7231)
src/Database/Adapter/Postgres.php (5)
src/Database/Adapter/MySQL.php (1)
handleDistanceSpatialQueries(92-120)src/Database/Adapter/MariaDB.php (1)
handleDistanceSpatialQueries(1365-1394)src/Database/Query.php (3)
Query(8-1051)getValues(164-167)getMethod(148-151)src/Database/Adapter/SQL.php (1)
convertArrayToWKT(2259-2298)src/Database/Database.php (1)
Database(37-7231)
⏰ 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). (11)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Unit Test
🔇 Additional comments (12)
src/Database/Adapter/MySQL.php (1)
98-120: Incorrect unit placement in ST_Distance and SRID mismatch risk
Pass the unit argument to ST_Distance(g1,g2,'meter')—not to ST_GeomFromText, which expects a numeric SRID—otherwise the SQL is invalid (dev.mysql.com). For meter distances on SRID-0 columns, use ST_Distance_Sphere to avoid SRID mismatch errors (dev.mysql.com).- $unit = $useMeters ? ", 'meter'" : ''; - - return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0){$unit}) {$operator} :{$placeholder}_1"; + if ($useMeters) { + // Use spherical distance for meter units on SRID-0 columns + return "ST_Distance_Sphere({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0), 6371000) {$operator} :{$placeholder}_1"; + } + return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0)) {$operator} :{$placeholder}_1";⛔ Skipped due to learnings
Learnt from: ArnabChatterjee20k PR: utopia-php/database#661 File: tests/e2e/Adapter/Scopes/SpatialTests.php:180-186 Timestamp: 2025-08-14T06:35:30.429Z Learning: Query::distance method in Utopia\Database\Query expects an array of values parameter, where each value is a [geometry, distance] pair. So the correct format is Query::distance('attribute', [[[lat, lng], distance]]) where the outer array contains the values and each value is [geometry, distance].src/Database/Adapter/MariaDB.php (1)
1417-1421: Good refactor: delegate distance cases to shared helper.This removes duplication and centralizes distance SQL generation.
src/Database/Adapter/Postgres.php (2)
1527-1527: Good refactor: centralized distance handlingRouting all distance predicates through a single helper simplifies maintenance.
1747-1756: Explicit SRID in geometry column types: LGTMDeclaring GEOMETRY(type, SRID) improves validation and operator planning.
src/Database/Validator/Spatial.php (1)
156-159: No action needed: encoder only processes arrays
The call toencodeSpatialDatainDatabase.phpis already guarded byis_array($node), so validated WKT strings never reach the encoder and there’s no contract mismatch.Likely an incorrect or invalid review comment.
src/Database/Database.php (1)
7138-7141: LGTM: spatial arrays now throw StructureException on invalid inputThis fixes the silent acceptance of bad spatial arrays. One question: this path bypasses the global $this->validate flag, i.e., it always validates. If that’s intentional, ignore; otherwise, gate with $this->validate.
tests/e2e/Adapter/Scopes/SpatialTests.php (3)
1556-1567: Great negative test for invalid polygon on updateDocuments.Correctly asserts StructureException is thrown when an invalid spatial structure is provided.
1987-2090: Robust invalid spatial types coverage.Good breadth across POINT/LINESTRING/POLYGON invalid shapes, including dimensionality and closure checks.
2092-2160: Meters-based distance tests look solid.Good use of meters=true to validate geodesic thresholds; dataset keeps lat≈0 to reduce distortion.
tests/unit/Validator/SpatialTest.php (3)
11-23: POINT validator tests look correct.Covers happy path, single coord, non-numeric, and nested array.
25-37: LINESTRING validator coverage is adequate.Valid multi-point cases and malformed point/arity checks are in place.
39-73: POLYGON validator tests are strong.Single-ring and multi-ring (hole) positives plus empties, not-closed, and non-numeric negatives.
| if ($meters) { | ||
| // Transform both attribute and input geometry to 3857 (meters) for distance calculation | ||
| $attr = "ST_Transform({$alias}.{$attribute}, 3857)"; | ||
| $geom = "ST_Transform(ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . "), 3857)"; | ||
| return "ST_Distance({$attr}, {$geom}) {$operator} :{$placeholder}_1"; | ||
| } | ||
|
|
||
| // Without meters, use the original SRID (e.g., 4326) | ||
| return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . ")) {$operator} :{$placeholder}_1"; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use geography for meter-accurate distances instead of 3857 transforms
EPSG:3857 introduces scale distortion; casting to geography provides meter-accurate results and aligns with MariaDB/MySQL semantics.
- if ($meters) {
- // Transform both attribute and input geometry to 3857 (meters) for distance calculation
- $attr = "ST_Transform({$alias}.{$attribute}, 3857)";
- $geom = "ST_Transform(ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . "), 3857)";
- return "ST_Distance({$attr}, {$geom}) {$operator} :{$placeholder}_1";
- }
+ if ($meters) {
+ // Accurate geodesic distance in meters
+ return "ST_Distance({$alias}.{$attribute}::geography, ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . ")::geography) {$operator} :{$placeholder}_1";
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($meters) { | |
| // Transform both attribute and input geometry to 3857 (meters) for distance calculation | |
| $attr = "ST_Transform({$alias}.{$attribute}, 3857)"; | |
| $geom = "ST_Transform(ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . "), 3857)"; | |
| return "ST_Distance({$attr}, {$geom}) {$operator} :{$placeholder}_1"; | |
| } | |
| // Without meters, use the original SRID (e.g., 4326) | |
| return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . ")) {$operator} :{$placeholder}_1"; | |
| } | |
| if ($meters) { | |
| // Accurate geodesic distance in meters | |
| return "ST_Distance({$alias}.{$attribute}::geography, ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . ")::geography) {$operator} :{$placeholder}_1"; | |
| } | |
| // Without meters, use the original SRID (e.g., 4326) | |
| return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . ")) {$operator} :{$placeholder}_1"; | |
| } |
🤖 Prompt for AI Agents
In src/Database/Adapter/Postgres.php around lines 1490 to 1499, the current
implementation projects geometries to EPSG:3857 for meter calculations which can
introduce distortion; instead, cast both the stored attribute and the input
geometry to PostGIS geography and call ST_Distance on those geography types so
distances are meter-accurate and consistent with MariaDB/MySQL semantics. Update
the meters branch to stop using ST_Transform(..., 3857) and instead cast the
attribute and the ST_GeomFromText(...) (using Database::SRID) to ::geography,
then compute ST_Distance between them; keep the alias, attribute name,
placeholders and the operator/parameter placeholder unchanged. Ensure the
non-meters branch remains using the original SRID as before.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Adapter/MariaDB.php (1)
1371-1371: Thanks for aligning the flag name with other adapters$useMeters matches MySQL’s naming and improves cross-adapter parity.
📜 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(2 hunks)src/Database/Adapter/MySQL.php(2 hunks)src/Database/Query.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Database/Adapter/MySQL.php
- src/Database/Query.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/MariaDB.php (4)
src/Database/Adapter/MySQL.php (1)
handleDistanceSpatialQueries(92-125)src/Database/Adapter/Postgres.php (1)
handleDistanceSpatialQueries(1465-1499)src/Database/Query.php (3)
Query(8-1051)getValues(164-167)getMethod(148-151)src/Database/Adapter/SQL.php (1)
convertArrayToWKT(2259-2298)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (1)
src/Database/Adapter/MariaDB.php (1)
1417-1421: Good centralization of distance logicDelegating all distance types to handleDistanceSpatialQueries removes duplication and simplifies future changes.
| protected function handleDistanceSpatialQueries(Query $query, array &$binds, string $attribute, string $alias, string $placeholder): string | ||
| { | ||
| $distanceParams = $query->getValues()[0]; | ||
| $binds[":{$placeholder}_0"] = $this->convertArrayToWKT($distanceParams[0]); | ||
| $binds[":{$placeholder}_1"] = $distanceParams[1]; | ||
|
|
||
| $useMeters = isset($distanceParams[2]) && $distanceParams[2] === true; | ||
|
|
||
| switch ($query->getMethod()) { | ||
| case Query::TYPE_DISTANCE_EQUAL: | ||
| $operator = '='; | ||
| break; | ||
| case Query::TYPE_DISTANCE_NOT_EQUAL: | ||
| $operator = '!='; | ||
| break; | ||
| case Query::TYPE_DISTANCE_GREATER_THAN: | ||
| $operator = '>'; | ||
| break; | ||
| case Query::TYPE_DISTANCE_LESS_THAN: | ||
| $operator = '<'; | ||
| break; | ||
| default: | ||
| throw new DatabaseException('Unknown spatial query method: ' . $query->getMethod()); | ||
| } | ||
|
|
||
| if ($useMeters) { | ||
| return "ST_DISTANCE_SPHERE({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0), 6371000) {$operator} :{$placeholder}_1"; | ||
| } | ||
| return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0)) {$operator} :{$placeholder}_1"; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Meters path likely POINT-only on MariaDB; avoid magic number and verify SRID handling
- ST_Distance_Sphere in MariaDB is generally POINT-only. Distance queries on LINESTRING/POLYGON with $useMeters=true will likely error. Either restrict meters-based distance to VAR_POINT in validation (preferred), or throw a clear DatabaseException here when attribute type isn’t a point.
- 6371000 is a magic number. Consider extracting an EARTH_RADIUS_METERS constant (e.g., Database::EARTH_RADIUS_METERS) to keep adapters in sync.
- SRID is not set on either side for MariaDB. If your stored geometries are lon/lat (WGS84), consider passing SRID explicitly to ST_GeomFromText (and, if supported, setting SRID on the column expression) for consistency with other adapters. Verify MariaDB’s setter semantics first.
Apply if acceptable:
- if ($useMeters) {
- return "ST_DISTANCE_SPHERE({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0), 6371000) {$operator} :{$placeholder}_1";
- }
- return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0)) {$operator} :{$placeholder}_1";
+ if ($useMeters) {
+ $attr = "{$alias}.{$attribute}"; // optionally: ST_SRID({$alias}.{$attribute}, " . Database::SRID . ")
+ $geom = "ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . ")"; // explicit SRID if inputs are WGS84
+ return "ST_Distance_Sphere({$attr}, {$geom}, " . (defined('Utopia\\Database\\Database::EARTH_RADIUS_METERS') ? Database::EARTH_RADIUS_METERS : 6371000) . ") {$operator} :{$placeholder}_1";
+ }
+ // keep unitless behavior when meters=false
+ return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0)) {$operator} :{$placeholder}_1";I can add a point-only validator path for MariaDB distance-with-meters and introduce Database::EARTH_RADIUS_METERS. Want me to open a follow-up PR?
🌐 Web query:
Does MariaDB support ST_Distance_Sphere(point, point, radius)? Is it restricted to POINT geometries, and from which versions?
💡 Result:
Short answer: Yes. MariaDB provides ST_DISTANCE_SPHERE(g1,g2[,r]) (returns spherical distance in meters, optional radius). It accepts POINT or MULTIPOINT geometries (not arbitrary geometry types). The function was added in the 10.2/10.3/10.4/10.5 branches (backports): available starting in 10.2.38, 10.3.29, 10.4.19 and 10.5.10 (and in later releases such as 10.6+). [1][2]
Sources:
- MariaDB ST_DISTANCE_SPHERE docs. [1]
- MariaDB release/change history / SKySQL doc listing versions added. [2]
Validate supported geometries, extract Earth-radius constant, and set SRID explicitly
- ST_Distance_Sphere only accepts POINT and MULTIPOINT (introduced in MariaDB 10.2.38/10.3.29/10.4.19/10.5.10+); invoking it on other geometry types will error—validate the column type before using
$useMetersor throw a clear DatabaseException. (mariadb.com) - Replace the magic number 6371000 with a shared constant (e.g.,
Database::EARTH_RADIUS_METERS) to keep adapters in sync. - Pass an explicit SRID to
ST_GeomFromTextand, if supported, apply it to the column expression for WGS84 consistency.
🤖 Prompt for AI Agents
In src/Database/Adapter/MariaDB.php around lines 1365–1394, validate the target
column geometry type before using ST_DISTANCE_SPHERE (only allow POINT or
MULTIPOINT) and throw a DatabaseException with a clear message if it’s
unsupported; replace the magic 6371000 with a shared constant (e.g.,
Database::EARTH_RADIUS_METERS) and pass an explicit SRID (use WGS84 4326) to
ST_GeomFromText, and where possible apply the same SRID to the column expression
so both operands share the WGS84 SRID; implement the column-type check by
querying the column metadata (or using the adapter’s schema info) and branch to
either the meters-enabled ST_DISTANCE_SPHERE path or the SRID-aware ST_Distance
path accordingly.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests