Sync 1.x #695
Sync 1.x #695
Conversation
updated spatial get type
reduced the spatial query dimension to avoid nested array
…ies in meters * Exprted a method from spatial validator to know the type of the spatial object
SpatialValidator update, distance in meter update
updated spatial update attribute
…e index validation * fixed long-lat order for geo function * added spatial types as filters * index validation updates * test update for index createion edge cases and filter encoding and decoding
Refactor spatial attribute handling(order, index , filters)
WalkthroughAdds a required flag to updateAttribute across adapters, introduces adapter capability getSupportForDistanceBetweenMultiDimensionGeometryInMeters(), refactors spatial distance handling and WKT parsing, moves spatial encode/decode into filters, tightens spatial index validation, adjusts Query spatial value wrapping, and expands spatial tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant DB as Database
participant Val as IndexValidator
participant Ad as Adapter
participant SQL as SQL Engine
Client->>DB: updateAttribute(collection, id, type, ..., required)
alt type is spatial
DB->>Val: checkSpatialIndex(collection, index..., adapterFlags)
Val-->>DB: pass/fail (IndexException on fail)
alt passes
DB->>Ad: updateAttribute(..., required)
Ad->>SQL: Execute ALTER/DDL (type-aware, NULL/NOT NULL for spatial)
SQL-->>Ad: result
Ad-->>DB: bool
DB-->>Client: bool
else fails
Val-->>Client: IndexException
end
else non-spatial
DB->>Ad: updateAttribute(..., required)
Ad-->>DB: bool
DB-->>Client: bool
end
sequenceDiagram
autonumber
actor Client
participant DB as Database
participant Ad as Adapter
participant SQL as SQL Engine
Client->>DB: Query(distanceWithinMeters(attr, geometry))
DB->>Ad: getSupportForDistanceBetweenMultiDimensionGeometryInMeters()
alt adapter supports multi-dim meters
DB->>Ad: handleDistanceSpatialQueries(query, binds, attribute, type, alias, ph)
note right of Ad: validate spatial types<br/>build ST_GeomFromText(... axis-order=long-lat/SRID or geography)
Ad->>SQL: Execute distance SQL
SQL-->>Ad: Rows
Ad-->>DB: Rows
DB-->>Client: Result
else not supported or invalid types
Ad-->>DB: Throw QueryException
DB-->>Client: Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/Database/Validator/Spatial.php (1)
113-116: Polygon closure check can fail on numeric stringsYou validate with is_numeric but compare whole point arrays strictly, so ['1','2'] !== [1,2] and valid rings are rejected. Compare coordinates numerically.
Apply:
- // Check that the ring is closed (first point == last point) - if ($ring[0] !== $ring[count($ring) - 1]) { + // Check that the ring is closed (first point == last point) using numeric comparison + $last = count($ring) - 1; + if ((float)$ring[0][0] !== (float)$ring[$last][0] || (float)$ring[0][1] !== (float)$ring[$last][1]) { $this->message = "Ring #{$ringIndex} must be closed (first point must equal last point)"; return false; }src/Database/Adapter/Postgres.php (1)
1516-1521: Bug: missing SRID in ST_GeomFromText for CROSSES/NOT_CROSSESOther spatial ops include SRID; these two don’t. This can yield SRID 0 geometries and wrong results.
Apply:
- 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 . "))";src/Database/Adapter/MySQL.php (1)
124-126: Add SRID to non-meters path to avoid SRID-mismatch errors.ST_GeomFromText without SRID returns SRID 0. If your column carries SRID 4326, ST_Distance may fail with ER_GIS_DIFFERENT_SRIDS. Bind the same SRID for consistency.
Apply:
- return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0)) {$operator} :{$placeholder}_1"; + return "ST_Distance({$alias}.{$attribute}, ST_GeomFromText(:{$placeholder}_0, " . Database::SRID . ")) {$operator} :{$placeholder}_1";Ref: SRID setting and mismatch behavior. (dev.mysql.com)
src/Database/Database.php (4)
1987-1993: Require spatial filters via contract (prevents silent misconfiguration).getRequiredFilters should also declare that spatial types require their matching filter. This both validates inputs and helps with updates where type changes to spatial.
Apply:
protected function getRequiredFilters(?string $type): array { return match ($type) { self::VAR_DATETIME => ['datetime'], + self::VAR_POINT, + self::VAR_LINESTRING, + self::VAR_POLYGON => [$type], default => [], }; }
2037-2047: Bug: fall-through throws “Unknown attribute type” for spatial defaults.Spatial cases don’t
break;after validating arrays, so they always fall through todefaultand throw. This will reject valid defaults for point/linestring/polygon.Apply:
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);
2330-2341: Typo: method name is setAttribute, not setattribute (fatal at runtime).This will cause a call to undefined method on Document.
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
7283-7327: decodeSpatialData: make parsing resilient (SRID prefix, Z/M coords, whitespace).Current parsing fails for common variants like
SRID=4326;POINT(...),POINT Z (...), extra spaces, or spaced ring separators. Prefer regex-based parsing and robust splitting.Apply:
- public function decodeSpatialData(string $wkt): array - { - $upper = strtoupper($wkt); - - // POINT(x y) - if (str_starts_with($upper, 'POINT(')) { - $start = strpos($wkt, '(') + 1; - $end = strrpos($wkt, ')'); - $inside = substr($wkt, $start, $end - $start); - - $coords = explode(' ', trim($inside)); - return [(float)$coords[0], (float)$coords[1]]; - } - - // LINESTRING(x1 y1, x2 y2, ...) - if (str_starts_with($upper, 'LINESTRING(')) { - $start = strpos($wkt, '(') + 1; - $end = strrpos($wkt, ')'); - $inside = substr($wkt, $start, $end - $start); - - $points = explode(',', $inside); - return array_map(function ($point) { - $coords = explode(' ', trim($point)); - return [(float)$coords[0], (float)$coords[1]]; - }, $points); - } - - // POLYGON((x1,y1),(x2,y2)) - if (str_starts_with($upper, 'POLYGON((')) { - $start = strpos($wkt, '((') + 2; - $end = strrpos($wkt, '))'); - $inside = substr($wkt, $start, $end - $start); - - $rings = explode('),(', $inside); - return array_map(function ($ring) { - $points = explode(',', $ring); - return array_map(function ($point) { - $coords = explode(' ', trim($point)); - return [(float)$coords[0], (float)$coords[1]]; - }, $points); - }, $rings); - } - - return [$wkt]; - } + public function decodeSpatialData(string $wkt): array + { + $wkt = \trim($wkt); + // Strip optional SRID prefix: SRID=4326;WKT + if (\strncasecmp($wkt, 'SRID=', 5) === 0) { + $semi = \strpos($wkt, ';'); + if ($semi !== false) { + $wkt = \substr($wkt, $semi + 1); + } + } + + // POINT [Z|M] + if (\preg_match('/^POINT(?:\s+Z|\s+M|Z|M)?\s*\(\s*([-+.\dEe]+)\s+([-+.\dEe]+)(?:\s+[-+.\dEe]+)?\s*\)\s*$/i', $wkt, $m)) { + return [(float)$m[1], (float)$m[2]]; + } + + // LINESTRING [Z|M] + if (\preg_match('/^LINESTRING(?:\s+Z|\s+M|Z|M)?\s*\(\s*(.+)\s*\)\s*$/i', $wkt, $m)) { + $pts = \preg_split('/\s*,\s*/', $m[1]); + return \array_map(function (string $pt) { + if (\preg_match('/^\s*([-+.\dEe]+)\s+([-+.\dEe]+)(?:\s+[-+.\dEe]+)?\s*$/', $pt, $mm)) { + return [(float)$mm[1], (float)$mm[2]]; + } + return [0.0, 0.0]; + }, $pts); + } + + // POLYGON [Z|M] + if (\preg_match('/^POLYGON(?:\s+Z|\s+M|Z|M)?\s*\(\s*(.+)\s*\)\s*$/i', $wkt, $m)) { + // Split rings: (...),(...) + $inside = \trim($m[1]); + $ringsText = \preg_split('/\)\s*,\s*\(/', \trim($inside, '()')) ?: []; + return \array_map(function (string $ring) { + $pts = \preg_split('/\s*,\s*/', $ring) ?: []; + return \array_map(function (string $pt) { + if (\preg_match('/^\s*([-+.\dEe]+)\s+([-+.\dEe]+)(?:\s+[-+.\dEe]+)?\s*$/', $pt, $mm)) { + return [(float)$mm[1], (float)$mm[2]]; + } + return [0.0, 0.0]; + }, $pts); + }, $ringsText); + } + + // Unknown geometry: return as-is in a one-element array for backward-compat + return [$wkt]; + }
🧹 Nitpick comments (10)
src/Database/Validator/Spatial.php (1)
126-130: Broaden WKT detection (optional)Consider supporting dimension tokens and optional SRID (EWKT) for robustness.
- return (bool) preg_match('/^(POINT|LINESTRING|POLYGON)\s*\(/i', $value); + return (bool) preg_match('/^(?:SRID=\d+;)?\s*(POINT|LINESTRING|POLYGON)(?:\s+(?:Z|M|ZM))?\s*\(/i', $value);src/Database/Adapter.php (1)
1081-1087: Naming/doc nit: “multidimension” → “multidimensional”Minor grammar tweak; also consider a shorter, clearer name (e.g., getSupportForMultiGeomDistanceMeters) for readability, if API churn allows.
src/Database/Adapter/SQLite.php (1)
330-336: Suppress unused-parameter noise in SQLite’s updateAttributeSQLite ignores most params here; static analysis flags them. Unset to keep tools quiet.
Apply:
public function updateAttribute(string $collection, string $id, string $type, int $size, bool $signed = true, bool $array = false, ?string $newKey = null, bool $required = false): bool { + // SQLite does not use these parameters for in-place alters + unset($type, $size, $signed, $array, $required); if (!empty($newKey) && $newKey !== $id) { return $this->renameAttribute($collection, $id, $newKey); } return true; }src/Database/Adapter/Postgres.php (1)
196-201: Minor: duplicate variable block
$attributeStringsis declared twice back-to-back. Remove the duplicate.- /** @var array<string> $attributeStrings */ - $attributeStrings = []; - - /** @var array<string> $attributeStrings */ - $attributeStrings = []; + /** @var array<string> $attributeStrings */ + $attributeStrings = [];src/Database/Adapter/MySQL.php (1)
88-93: $type is unused; suppress PHPMD or document intent.Keep the parameter to honor the inherited signature, but silence the warning.
Add a suppression:
/** * Handle distance spatial queries * + * @SuppressWarnings(PHPMD.UnusedFormalParameter) * @param Query $query * @param array<string, mixed> $binds * @param string $attribute * @param string $typesrc/Database/Validator/Index.php (1)
364-367: Clarify error text for non-spatial index on spatial attribute.Current wording implies spatial-only, yet we’re rejecting a non-spatial index. Suggest clearer language.
- $this->message = 'Spatial index can only be created on spatial attributes (point, linestring, polygon). Attribute "' . $attributeName . '" is of type "' . $attributeType . '"'; + $this->message = 'Non-spatial indexes cannot be created on spatial attributes. Attribute "' . $attributeName . '" is of type "' . $attributeType . '"';tests/e2e/Adapter/Scopes/SpatialTests.php (1)
15-15: Validator import present.If unused after final edits, consider dropping later to keep imports lean. No action required now.
src/Database/Database.php (3)
1327-1339: Auto-wiring spatial filters on createCollection looks good; consider migration/backfill for existing metadata.This ensures new spatial attributes encode/decode via filters. However, existing collections created before this change may lack these filters and will stop encoding spatial arrays to WKT. Recommend a migration/backfill path or an automatic safeguard (see suggested changes to getRequiredFilters and updateAttribute below).
I can draft a metadata migration script to append the missing spatial filters to existing attributes across collections. Do you want that?
2263-2267: Only force structural ALTER when required actually changes for spatial types.Current logic marks
$altering = truefor any spatial attribute regardless of changes, which can trigger unnecessary DDL. Gate this on a required-nullability transition when the adapter cannot index null spatial values.- // we need to alter table attribute type to NOT NULL/NULL for change in required - if (!$this->adapter->getSupportForSpatialIndexNull() && in_array($type, Database::SPATIAL_TYPES)) { - $altering = true; - } + // If adapter can't index NULLs for spatial types, ALTER only if nullability changes + if (!$this->adapter->getSupportForSpatialIndexNull() && \in_array($type, Database::SPATIAL_TYPES, true)) { + $previousRequired = (bool)$attribute->getAttribute('required'); + if ($required !== $previousRequired) { + $altering = true; + } + } + + // Ensure spatial filters remain present after type changes + if (\in_array($type, self::SPATIAL_TYPES, true) && !\in_array($type, (array)$filters, true)) { + $filters = \array_values(\array_unique([...((array)$filters), $type])); + }
7226-7274: encodeSpatialData: OK; ensure polygon rings are validated as closed in Spatial validator.Assuming Utopia\Database\Validator\Spatial enforces ring closure and min points, this encoder is fine. If not, add a check to close rings to avoid invalid WKT on some engines.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/MariaDB.php(8 hunks)src/Database/Adapter/MySQL.php(3 hunks)src/Database/Adapter/Pool.php(2 hunks)src/Database/Adapter/Postgres.php(3 hunks)src/Database/Adapter/SQL.php(1 hunks)src/Database/Adapter/SQLite.php(2 hunks)src/Database/Database.php(6 hunks)src/Database/Query.php(8 hunks)src/Database/Validator/Index.php(1 hunks)src/Database/Validator/Spatial.php(1 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(11 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/Adapter/Postgres.phptests/e2e/Adapter/Scopes/SpatialTests.phpsrc/Database/Adapter/MariaDB.php
🧬 Code graph analysis (9)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-7328)
src/Database/Adapter/MySQL.php (5)
src/Database/Adapter/MariaDB.php (2)
handleDistanceSpatialQueries(1367-1402)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1912-1915)src/Database/Adapter/Postgres.php (2)
handleDistanceSpatialQueries(1466-1499)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1991-1994)src/Database/Adapter.php (1)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1086-1086)src/Database/Adapter/Pool.php (1)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(538-541)src/Database/Adapter/SQLite.php (1)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1273-1276)
src/Database/Adapter/Pool.php (7)
src/Database/Database.php (1)
updateAttribute(2241-2440)src/Database/Adapter/MariaDB.php (2)
updateAttribute(417-438)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1912-1915)src/Database/Adapter.php (2)
updateAttribute(585-585)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1086-1086)src/Database/Adapter/Postgres.php (2)
updateAttribute(543-589)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1991-1994)src/Database/Adapter/SQLite.php (2)
updateAttribute(335-342)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1273-1276)src/Database/Mirror.php (2)
updateAttribute(393-444)delegate(88-103)src/Database/Adapter/MySQL.php (1)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(183-186)
src/Database/Adapter.php (7)
src/Database/Database.php (1)
updateAttribute(2241-2440)src/Database/Adapter/MariaDB.php (2)
updateAttribute(417-438)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1912-1915)src/Database/Adapter/Pool.php (2)
updateAttribute(178-181)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(538-541)src/Database/Adapter/Postgres.php (2)
updateAttribute(543-589)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1991-1994)src/Database/Adapter/SQLite.php (2)
updateAttribute(335-342)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1273-1276)src/Database/Mirror.php (1)
updateAttribute(393-444)src/Database/Adapter/MySQL.php (1)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(183-186)
src/Database/Adapter/Postgres.php (6)
src/Database/Adapter/MariaDB.php (3)
updateAttribute(417-438)getSQLType(1594-1681)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1912-1915)src/Database/Adapter.php (3)
updateAttribute(585-585)filter(1183-1192)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1086-1086)src/Database/Adapter/Pool.php (2)
updateAttribute(178-181)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(538-541)src/Database/Adapter/SQLite.php (2)
updateAttribute(335-342)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1273-1276)src/Database/Mirror.php (1)
updateAttribute(393-444)src/Database/Adapter/MySQL.php (1)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(183-186)
tests/e2e/Adapter/Scopes/SpatialTests.php (8)
src/Database/Database.php (2)
Database(37-7328)createIndex(3235-3390)src/Database/Validator/Index.php (1)
Index(10-383)src/Database/Query.php (9)
Query(8-1075)intersects(987-990)notIntersects(999-1002)overlaps(1035-1038)notOverlaps(1047-1050)notTouches(1071-1074)distanceLessThan(975-978)distanceGreaterThan(961-964)distanceEqual(933-936)src/Database/Adapter/MariaDB.php (4)
createIndex(715-778)getSupportForSpatialAttributes(1873-1876)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1912-1915)getSupportForSpatialIndexNull(1883-1886)src/Database/Adapter.php (4)
createIndex(671-671)getSupportForSpatialAttributes(1058-1058)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1086-1086)getSupportForSpatialIndexNull(1065-1065)src/Database/Adapter/Postgres.php (4)
createIndex(841-901)getSupportForSpatialAttributes(1951-1954)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1991-1994)getSupportForSpatialIndexNull(1961-1964)src/Database/Adapter/SQLite.php (4)
createIndex(455-480)getSupportForSpatialAttributes(971-974)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1273-1276)getSupportForSpatialIndexNull(976-979)src/Database/Adapter/MySQL.php (1)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(183-186)
src/Database/Database.php (7)
src/Database/Adapter/MariaDB.php (2)
getSupportForSpatialIndexNull(1883-1886)updateAttribute(417-438)src/Database/Adapter.php (2)
getSupportForSpatialIndexNull(1065-1065)updateAttribute(585-585)src/Database/Adapter/Pool.php (2)
getSupportForSpatialIndexNull(433-436)updateAttribute(178-181)src/Database/Adapter/Postgres.php (2)
getSupportForSpatialIndexNull(1961-1964)updateAttribute(543-589)src/Database/Adapter/SQLite.php (2)
getSupportForSpatialIndexNull(976-979)updateAttribute(335-342)src/Database/Adapter/SQL.php (1)
getSupportForSpatialIndexNull(1501-1504)src/Database/Mirror.php (1)
updateAttribute(393-444)
src/Database/Adapter/MariaDB.php (6)
src/Database/Database.php (2)
Database(37-7328)updateAttribute(2241-2440)src/Database/Query.php (3)
Query(8-1075)getValues(164-167)getMethod(148-151)src/Database/Adapter.php (3)
updateAttribute(585-585)getSupportForSpatialIndexNull(1065-1065)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1086-1086)src/Database/Adapter/Postgres.php (4)
updateAttribute(543-589)handleDistanceSpatialQueries(1466-1499)getSupportForSpatialIndexNull(1961-1964)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1991-1994)src/Database/Adapter/SQL.php (3)
convertArrayToWKT(2259-2298)getSpatialTypeFromWKT(2661-2669)getSupportForSpatialIndexNull(1501-1504)src/Database/Adapter/MySQL.php (2)
handleDistanceSpatialQueries(93-126)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(183-186)
src/Database/Adapter/SQLite.php (7)
src/Database/Database.php (1)
updateAttribute(2241-2440)src/Database/Adapter/MariaDB.php (2)
updateAttribute(417-438)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1912-1915)src/Database/Adapter.php (2)
updateAttribute(585-585)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1086-1086)src/Database/Adapter/Pool.php (2)
updateAttribute(178-181)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(538-541)src/Database/Adapter/Postgres.php (2)
updateAttribute(543-589)getSupportForDistanceBetweenMultiDimensionGeometryInMeters(1991-1994)src/Database/Mirror.php (1)
updateAttribute(393-444)src/Database/Adapter/MySQL.php (1)
getSupportForDistanceBetweenMultiDimensionGeometryInMeters(183-186)
🪛 PHPMD (2.15.0)
src/Database/Adapter/MySQL.php
93-93: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
src/Database/Adapter/SQLite.php
335-335: Avoid unused parameters such as '$type'. (undefined)
(UnusedFormalParameter)
335-335: Avoid unused parameters such as '$size'. (undefined)
(UnusedFormalParameter)
335-335: Avoid unused parameters such as '$signed'. (undefined)
(UnusedFormalParameter)
335-335: Avoid unused parameters such as '$array'. (undefined)
(UnusedFormalParameter)
335-335: Avoid unused parameters such as '$required'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (43)
src/Database/Validator/Spatial.php (1)
147-150: Getter addition LGTMPublic accessor is straightforward and useful for callers needing the declared spatial type.
src/Database/Adapter.php (1)
581-586: updateAttribute signature change — implementations & calls verified
- Abstract method at src/Database/Adapter.php:585 includes final bool $required and all Adapter implementations match (Postgres: src/Database/Adapter/Postgres.php:543; MariaDB: src/Database/Adapter/MariaDB.php:417; SQLite: src/Database/Adapter/SQLite.php:335; Pool: src/Database/Adapter/Pool.php:178).
- Database calls adapter with the correct argument order (src/Database/Database.php:2429).
- Mirror forwards to Database::updateAttribute (src/Database/Mirror.php:393).
src/Database/Query.php (2)
989-989: Correct: spatial helpers now pass a single geometry value via nested arrayWrapping
$valuesas[$values]aligns these helpers with adapter expectations (getValues()[0]→ geometry). Good fix.Also applies to: 1001-1001, 1013-1013, 1025-1025, 1037-1037, 1049-1049, 1061-1061, 1073-1073
989-989: Compatibility check — spatial helpers now nest values; verify tests & external JSON consumers
- Repo scan: spatial handlers already index the nested shape (use getValues()[0]) in src/Database/Adapter/Postgres.php, src/Database/Adapter/MariaDB.php and src/Database/Adapter/MySQL.php — I found no remaining internal spatial call sites that read a flat values shape.
- Action: Run unit/integration tests and confirm any external consumers (saved/serialized Query JSON, APIs, adapters outside this repo) handle the new nested values shape.
src/Database/Adapter/Pool.php (2)
178-181: API synced: updateAttribute now forwards $requiredSignature matches Adapter abstract and delegates correctly.
533-541: New capability passthrough wired
getSupportForDistanceBetweenMultiDimensionGeometryInMeters()correctly delegates to underlying adapter.src/Database/Adapter/SQLite.php (1)
1269-1276: Capability exposed
getSupportForDistanceBetweenMultiDimensionGeometryInMeters()returning false for SQLite is appropriate.src/Database/Adapter/Postgres.php (3)
538-549: API synced: updateAttribute includes $required and passes it to getSQLTypeSignature and propagation look correct.
1492-1494: Meters path correctly uses geographyCasting both sides to geography before ST_Distance is correct for meters.
1986-1994: Capability advertised
getSupportForDistanceBetweenMultiDimensionGeometryInMeters()returning true for Postgres makes sense.src/Database/Adapter/MySQL.php (2)
118-122: Meters path: axis-order hint and 'metre' unit are correct for MySQL 8.0+.Using ST_GeomFromText(..., SRID, 'axis-order=long-lat') and ST_Distance(..., 'metre') aligns with MySQL’s axis-order override and supported unit names. Looks good. (dev.mysql.com)
178-186: Capability method returning true is appropriate.MySQL supports distances in meters for non-point geometries in modern versions; exposing this via the adapter matches the test gating strategy. (dev.mysql.com)
src/Database/Validator/Index.php (2)
351-352: Non-spatial attributes correctly skipped.Switching to per-attribute evaluation is fine and avoids false positives when no spatial fields are present.
354-357: Good: early exit when adapter lacks spatial index support.Clear message and behavior look right.
tests/e2e/Adapter/Scopes/SpatialTests.php (15)
8-9: Exception aliases imported—good for specificity in assertions.
141-143: Intersects/notIntersects argument shape fixed (point value).Passing [x,y] matches Query helpers’ wrapping.
157-159: Intersects/notIntersects for linestring now use point input—correct.
188-194: Intersects/overlaps inputs normalized for polygon tests—looks right.
985-990: Compound spatial logic reads well (intersects + notTouches).
1095-1095: Fallback to intersects when contains not supported—sensible gating.
1142-1149: Triangle point-intersection assertions: good coverage of positive/negative cases.
1212-1213: Intersects with line geometry—good additional shape coverage.
1234-1241: Intersections against linestring (point and line)—solid.
2209-2337: New: distance-in-meters for multi-dimension geometries.Well-gated by adapter capability; cases exercise polygon↔polygon, point↔polygon, and equality at 0m.
2339-2393: New: error-path tests for adapters without multi-dimension meter support.Good negative coverage, including message content checks.
2395-2462: New: encode/decode round-trips for spatial types.Covers array and WKT inputs and nulls. Nice addition.
2463-2513: New: enforce single-attribute spatial index and type restrictions.Validates mixed-type and wrong-type index creation paths.
2515-2548: New: required toggling flow for spatial indexes on adapters without NULL support.Accurately models the expected failure-then-success sequence.
2550-2610: New: spatial index validations on non-spatial attributes and vice versa.Thorough matrix of invalid combinations.
src/Database/Database.php (2)
2354-2381: Spatial index nullability guard is solid.Good pre-check to enforce required=true for spatially indexed attributes when the adapter cannot support NULLs, and correct exception type (IndexException).
3320-3341: Correct exception type for spatial index validation.Switching to IndexException for “attribute not found” and “non-spatial attribute” under spatial indexes is appropriate and aligns error taxonomy.
src/Database/Adapter/MariaDB.php (12)
12-12: Import added for QueryException.The import for
QueryExceptionis correctly added to support the new spatial query validation logic.
413-417: updateAttribute signature extended with required parameter.The method signature now includes the
$requiredparameter to support required field constraints. This aligns with the abstract method signature in the baseAdapterclass.
422-422: Required parameter passed to getSQLType.The
$requiredparameter is correctly threaded through togetSQLType()to handle nullability constraints for spatial types.
394-398: Enhanced validation for distance-in-meters queries.The validation logic correctly enforces that distance-in-meters calculations are only supported between POINT geometries. The error message is clear and informative.
370-371: WKT conversion and binding properly handled.The code correctly converts geometry arrays to WKT format and binds the values for SQL execution.
1420-1470: Spatial functions enhanced with axis-order specification.All spatial functions now include
'axis-order=long-lat'parameter inST_GeomFromText()calls, which standardizes coordinate interpretation and improves spatial query accuracy.
1430-1430: Type parameter correctly passed to distance handler.The
$typeparameter is properly passed tohandleDistanceSpatialQueries()to enable type validation.
1499-1499: Type parameter added to spatial query handler.The method signature now accepts a
$typeparameter to support type-aware spatial query handling.
1644-1676: Spatial SQL types enhanced with nullability constraints.The spatial types (POINT, LINESTRING, POLYGON) now properly handle nullability based on the
$requiredparameter and spatial index null support. This ensures consistent behavior across different spatial configurations.
907-915: New capability method for distance-in-meters support.The method correctly returns
falsefor MariaDB, indicating that distance calculations in meters between multi-dimensional geometries are not supported by this adapter.
1594-1594: getSQLType method signature updated.The method signature correctly includes the
$requiredparameter to support nullability constraints in SQL type generation.
362-367: Distance-in-meters validation verified — point↔point enforcedhandleDistanceSpatialQueries in src/Database/Adapter/MariaDB.php uses getSpatialTypeFromWKT($wkt) and strtolower($type) and throws a QueryException unless both are Database::VAR_POINT; e2e tests (spatial_distance_meters_ and spatial_distance_error_test) cover the allowed and error paths.
| public function getSpatialTypeFromWKT(string $wkt): string | ||
| { | ||
| $wkt = trim($wkt); | ||
| $pos = strpos($wkt, '('); | ||
| if ($pos === false) { | ||
| throw new DatabaseException("Invalid spatial type"); | ||
| } | ||
| return strtolower(trim(substr($wkt, 0, $pos))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize WKT type and handle Z/M/SRID tokens
Current parsing returns the raw pre-“(” token lowercased. It will mis-handle inputs like “POINT Z (...)” or EWKT “SRID=4326;POINT (...)”, and may not map to Database::VAR_* constants (e.g., 'linestring' vs VAR_LINESTRING). Canonicalize via regex and map to VAR_*.
- public function getSpatialTypeFromWKT(string $wkt): string
- {
- $wkt = trim($wkt);
- $pos = strpos($wkt, '(');
- if ($pos === false) {
- throw new DatabaseException("Invalid spatial type");
- }
- return strtolower(trim(substr($wkt, 0, $pos)));
- }
+ public function getSpatialTypeFromWKT(string $wkt): string
+ {
+ $wkt = trim($wkt);
+ if (!preg_match('/^(?:SRID=\d+;)?\s*([A-Z]+)(?:\s+(?:Z|M|ZM))?\s*\(/i', $wkt, $m)) {
+ throw new DatabaseException('Invalid spatial type in WKT');
+ }
+ $type = strtolower($m[1]);
+ $map = [
+ 'point' => Database::VAR_POINT,
+ 'linestring' => Database::VAR_LINESTRING,
+ 'polygon' => Database::VAR_POLYGON,
+ ];
+ if (!isset($map[$type])) {
+ throw new DatabaseException('Unsupported geometry type: ' . $type);
+ }
+ return $map[$type];
+ }📝 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.
| public function getSpatialTypeFromWKT(string $wkt): string | |
| { | |
| $wkt = trim($wkt); | |
| $pos = strpos($wkt, '('); | |
| if ($pos === false) { | |
| throw new DatabaseException("Invalid spatial type"); | |
| } | |
| return strtolower(trim(substr($wkt, 0, $pos))); | |
| } | |
| public function getSpatialTypeFromWKT(string $wkt): string | |
| { | |
| $wkt = trim($wkt); | |
| if (!preg_match('/^(?:SRID=\d+;)?\s*([A-Z]+)(?:\s+(?:Z|M|ZM))?\s*\(/i', $wkt, $m)) { | |
| throw new DatabaseException('Invalid spatial type in WKT'); | |
| } | |
| $type = strtolower($m[1]); | |
| $map = [ | |
| 'point' => Database::VAR_POINT, | |
| 'linestring' => Database::VAR_LINESTRING, | |
| 'polygon' => Database::VAR_POLYGON, | |
| ]; | |
| if (!isset($map[$type])) { | |
| throw new DatabaseException('Unsupported geometry type: ' . $type); | |
| } | |
| return $map[$type]; | |
| } |
|
|
||
| self::addFilter( | ||
| Database::VAR_POINT, | ||
| /** | ||
| * @param mixed $value | ||
| * @return mixed | ||
| */ | ||
| function (mixed $value) { | ||
| if (!is_array($value)) { | ||
| return $value; | ||
| } | ||
| try { | ||
| return self::encodeSpatialData($value, Database::VAR_POINT); | ||
| } catch (\Throwable) { | ||
| return $value; | ||
| } | ||
| }, | ||
| /** | ||
| * @param string|null $value | ||
| * @return string|null | ||
| */ | ||
| function (?string $value) { | ||
| if (!is_string($value)) { | ||
| return $value; | ||
| } | ||
| return self::decodeSpatialData($value); | ||
| } | ||
| ); | ||
| self::addFilter( | ||
| Database::VAR_LINESTRING, | ||
| /** | ||
| * @param mixed $value | ||
| * @return mixed | ||
| */ | ||
| function (mixed $value) { | ||
| if (is_null($value)) { | ||
| return; | ||
| } | ||
| try { | ||
| return self::encodeSpatialData($value, Database::VAR_LINESTRING); | ||
| } catch (\Throwable) { | ||
| return $value; | ||
| } | ||
| }, | ||
| /** | ||
| * @param string|null $value | ||
| * @return string|null | ||
| */ | ||
| function (?string $value) { | ||
| if (is_null($value)) { | ||
| return $value; | ||
| } | ||
| return self::decodeSpatialData($value); | ||
| } | ||
| ); | ||
| self::addFilter( | ||
| Database::VAR_POLYGON, | ||
| /** | ||
| * @param mixed $value | ||
| * @return mixed | ||
| */ | ||
| function (mixed $value) { | ||
| if (is_null($value)) { | ||
| return; | ||
| } | ||
| try { | ||
| return self::encodeSpatialData($value, Database::VAR_POLYGON); | ||
| } catch (\Throwable) { | ||
| return $value; | ||
| } | ||
| }, | ||
| /** | ||
| * @param string|null $value | ||
| * @return string|null | ||
| */ | ||
| function (?string $value) { | ||
| if (is_null($value)) { | ||
| return $value; | ||
| } | ||
| return self::decodeSpatialData($value); | ||
| } | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Spatial filters: unify encode guards and fix docblocks (also remove stray double-space).
- Encode closures for linestring/polygon should mirror point’s guard: only call encodeSpatialData when the incoming value is an array; otherwise return the value unchanged. Today they return null on null, but will pass strings to the encoder (likely failing validation). Also, docblocks claim string|null yet decode returns arrays; make them mixed to avoid misleading PHPStan and readers. Minor: remove the double-space before
self::encodeSpatialData.
Apply:
@@
- /**
- * @param string|null $value
- * @return string|null
- */
- function (?string $value) {
+ /**
+ * @param mixed $value
+ * @return mixed
+ */
+ function (mixed $value) {
if (!is_string($value)) {
return $value;
}
return self::decodeSpatialData($value);
}
@@
- function (mixed $value) {
- if (is_null($value)) {
- return;
- }
+ function (mixed $value) {
+ if (!\is_array($value)) {
+ return $value;
+ }
try {
- return self::encodeSpatialData($value, Database::VAR_LINESTRING);
+ return self::encodeSpatialData($value, Database::VAR_LINESTRING);
} catch (\Throwable) {
return $value;
}
},
@@
- /**
- * @param string|null $value
- * @return string|null
- */
- function (?string $value) {
- if (is_null($value)) {
- return $value;
- }
+ /**
+ * @param mixed $value
+ * @return mixed
+ */
+ function (mixed $value) {
+ if (!\is_string($value)) {
+ return $value;
+ }
return self::decodeSpatialData($value);
}
@@
- function (mixed $value) {
- if (is_null($value)) {
- return;
- }
+ function (mixed $value) {
+ if (!\is_array($value)) {
+ return $value;
+ }
try {
- return self::encodeSpatialData($value, Database::VAR_POLYGON);
+ return self::encodeSpatialData($value, Database::VAR_POLYGON);
} catch (\Throwable) {
return $value;
}
},
@@
- /**
- * @param string|null $value
- * @return string|null
- */
- function (?string $value) {
- if (is_null($value)) {
- return $value;
- }
+ /**
+ * @param mixed $value
+ * @return mixed
+ */
+ function (mixed $value) {
+ if (!\is_string($value)) {
+ return $value;
+ }
return self::decodeSpatialData($value);
}📝 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.
| self::addFilter( | |
| Database::VAR_POINT, | |
| /** | |
| * @param mixed $value | |
| * @return mixed | |
| */ | |
| function (mixed $value) { | |
| if (!is_array($value)) { | |
| return $value; | |
| } | |
| try { | |
| return self::encodeSpatialData($value, Database::VAR_POINT); | |
| } catch (\Throwable) { | |
| return $value; | |
| } | |
| }, | |
| /** | |
| * @param string|null $value | |
| * @return string|null | |
| */ | |
| function (?string $value) { | |
| if (!is_string($value)) { | |
| return $value; | |
| } | |
| return self::decodeSpatialData($value); | |
| } | |
| ); | |
| self::addFilter( | |
| Database::VAR_LINESTRING, | |
| /** | |
| * @param mixed $value | |
| * @return mixed | |
| */ | |
| function (mixed $value) { | |
| if (is_null($value)) { | |
| return; | |
| } | |
| try { | |
| return self::encodeSpatialData($value, Database::VAR_LINESTRING); | |
| } catch (\Throwable) { | |
| return $value; | |
| } | |
| }, | |
| /** | |
| * @param string|null $value | |
| * @return string|null | |
| */ | |
| function (?string $value) { | |
| if (is_null($value)) { | |
| return $value; | |
| } | |
| return self::decodeSpatialData($value); | |
| } | |
| ); | |
| self::addFilter( | |
| Database::VAR_POLYGON, | |
| /** | |
| * @param mixed $value | |
| * @return mixed | |
| */ | |
| function (mixed $value) { | |
| if (is_null($value)) { | |
| return; | |
| } | |
| try { | |
| return self::encodeSpatialData($value, Database::VAR_POLYGON); | |
| } catch (\Throwable) { | |
| return $value; | |
| } | |
| }, | |
| /** | |
| * @param string|null $value | |
| * @return string|null | |
| */ | |
| function (?string $value) { | |
| if (is_null($value)) { | |
| return $value; | |
| } | |
| return self::decodeSpatialData($value); | |
| } | |
| ); | |
| self::addFilter( | |
| Database::VAR_POINT, | |
| /** | |
| * @param mixed $value | |
| * @return mixed | |
| */ | |
| function (mixed $value) { | |
| if (!is_array($value)) { | |
| return $value; | |
| } | |
| try { | |
| return self::encodeSpatialData($value, Database::VAR_POINT); | |
| } catch (\Throwable) { | |
| return $value; | |
| } | |
| }, | |
| /** | |
| * @param mixed $value | |
| * @return mixed | |
| */ | |
| function (mixed $value) { | |
| if (!is_string($value)) { | |
| return $value; | |
| } | |
| return self::decodeSpatialData($value); | |
| } | |
| ); | |
| self::addFilter( | |
| Database::VAR_LINESTRING, | |
| /** | |
| * @param mixed $value | |
| * @return mixed | |
| */ | |
| function (mixed $value) { | |
| if (!\is_array($value)) { | |
| return $value; | |
| } | |
| try { | |
| return self::encodeSpatialData($value, Database::VAR_LINESTRING); | |
| } catch (\Throwable) { | |
| return $value; | |
| } | |
| }, | |
| /** | |
| * @param mixed $value | |
| * @return mixed | |
| */ | |
| function (mixed $value) { | |
| if (!\is_string($value)) { | |
| return $value; | |
| } | |
| return self::decodeSpatialData($value); | |
| } | |
| ); | |
| self::addFilter( | |
| Database::VAR_POLYGON, | |
| /** | |
| * @param mixed $value | |
| * @return mixed | |
| */ | |
| function (mixed $value) { | |
| if (!\is_array($value)) { | |
| return $value; | |
| } | |
| try { | |
| return self::encodeSpatialData($value, Database::VAR_POLYGON); | |
| } catch (\Throwable) { | |
| return $value; | |
| } | |
| }, | |
| /** | |
| * @param mixed $value | |
| * @return mixed | |
| */ | |
| function (mixed $value) { | |
| if (!\is_string($value)) { | |
| return $value; | |
| } | |
| return self::decodeSpatialData($value); | |
| } | |
| ); |
| if (in_array($type, Database::SPATIAL_TYPES)) { | ||
| $filters[] = $type; | ||
| $filters = array_unique($filters); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good: spatial attributes gain their filter automatically on createAttribute.
This aligns the write path with the new filter-driven spatial handling. Pair this with required-filter enforcement (next comment) to avoid drift.
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 1696-1699, ensure spatial attribute
types are added to the attribute's filters on creation: append the spatial $type
to $filters, call array_unique($filters) to remove duplicates, and then assign
the resulting $filters back to the attribute (or the structure that will be
persisted/returned) so the updated filter list is actually saved; if $filters
may be null, initialize it as an array before appending.
| } | ||
|
|
||
| $updated = $this->adapter->updateAttribute($collection, $id, $type, $size, $signed, $array, $newKey); | ||
| $updated = $this->adapter->updateAttribute($collection, $id, $type, $size, $signed, $array, $newKey, $required); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Adapter signature change propagation: verify all adapters and call sites accept $required.
You updated the call to pass $required. Please verify all Adapter implementations and any proxies (Pool, Mirror, etc.) reflect the new signature.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "UpdateAttribute signatures in adapters:"
rg -nP 'function\s+updateAttribute\s*\(' src/Database/Adapter -C1
echo
echo "Calls to ->updateAttribute across src:"
rg -nP '\->updateAttribute\s*\(' -n -C2 src
echo
echo "Scan for incorrect setattribute() usages:"
rg -nP '\bsetattribute\s*\(' -n -C1 srcLength of output: 2766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Mirror.php (full file with line numbers) ==="
nl -ba src/Database/Mirror.php | sed -n '1,520p' || true
echo
echo "=== Pool.php (full file with line numbers) ==="
nl -ba src/Database/Adapter/Pool.php | sed -n '1,260p' || true
echo
echo "=== All ->updateAttribute call sites (with context) ==="
rg -nP '\->updateAttribute\s*\(' -n -C4 src || true
echo
echo "=== Adapter updateAttribute signatures ==="
rg -nP 'public function updateAttribute\(.*\)' src/Database/Adapter -n -C0 || trueLength of output: 3025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Mirror.php (lines 388-440):"
awk 'NR>=388 && NR<=440{printf "%4d: %s\n", NR, $0}' src/Database/Mirror.php || true
echo
echo "Pool.php (lines 168-190):"
awk 'NR>=168 && NR<=190{printf "%4d: %s\n", NR, $0}' src/Database/Adapter/Pool.php || true
echo
echo "Database.php (lines 2420-2432):"
awk 'NR>=2420 && NR<=2432{printf "%4d: %s\n", NR, $0}' src/Database/Database.php || true
echo
echo "All ->updateAttribute call sites with 10 lines context:"
rg -nP '\->updateAttribute\s*\(' -n -C10 src || trueLength of output: 8252
Fix Mirror::updateAttribute forwarding — adapters expect $required as the last parameter
- Adapters updated and include $required as last arg: src/Database/Adapter/SQLite.php:335, src/Database/Adapter/MariaDB.php:417, src/Database/Adapter/Postgres.php:543, src/Database/Adapter/Pool.php:178.
- Database call is correct: src/Database/Database.php:2429 passes ($collection, $id, $type, $size, $signed, $array, $newKey, $required).
- Issue: src/Database/Mirror.php forwards $required as the 5th parameter to source/destination (calls at ~lines 395–407 and 425–437), which mismatches adapter ordering and will fail when source/destination are adapters/pools.
- Action: Update Mirror::updateAttribute to map/forward arguments in the adapter-compatible order (e.g. $collection, $id, $type, $size, $signed, $array, $newKey, $required) or adjust the called interfaces so they match. Change the calls at src/Database/Mirror.php (lines ~395–407 and 425–437).
🤖 Prompt for AI Agents
In src/Database/Mirror.php around lines ~395–407 and ~425–437,
Mirror::updateAttribute is forwarding $required as the 5th parameter to
source/destination updateAttribute calls which mismatches the adapters that
expect $required as the last argument; update both call sites so the forwarded
argument order matches the adapter signature: call updateAttribute($collection,
$id, $type, $size, $signed, $array, $newKey, $required) for both source and
destination invocations.
| if (count($attributes) !== 1) { | ||
| $this->message = 'Spatial index can be created on a single spatial attribute'; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate “single spatial attribute” check to actual spatial indexes.
Using count($attributes) against the whole list may emit this message even for non-spatial indexes that merely include a spatial attribute. Tie this rule to INDEX_SPATIAL (or a hasSpatial flag) for precision.
- if (count($attributes) !== 1) {
- $this->message = 'Spatial index can be created on a single spatial attribute';
- return false;
- }
+ if ($type === Database::INDEX_SPATIAL && count($attributes) !== 1) {
+ $this->message = 'Spatial index can be created on a single spatial attribute';
+ return false;
+ }📝 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 (count($attributes) !== 1) { | |
| $this->message = 'Spatial index can be created on a single spatial attribute'; | |
| return false; | |
| } | |
| if ($type === Database::INDEX_SPATIAL && count($attributes) !== 1) { | |
| $this->message = 'Spatial index can be created on a single spatial attribute'; | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/Database/Validator/Index.php around lines 359 to 362, the validator
unconditionally enforces a single-attribute rule using count($attributes), which
can produce the "Spatial index can be created on a single spatial attribute"
error for non-spatial indexes; change the check so it only runs when the index
is actually a spatial index (e.g. INDEX_SPATIAL or a hasSpatial flag on the
index definition). Specifically, gate the count($attributes) !== 1 check behind
a condition that verifies the index type is spatial, and only set the
message/return false in that branch.
| if (!empty($orders) && !$this->spatialIndexOrderSupport) { | ||
| $this->message = 'Spatial indexes with explicit orders are not supported. Remove the orders to create this index.'; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Bug: order-check applies to all indexes, not only spatial.
As written, any index with orders on an adapter lacking spatial-index-order support will fail, even for non-spatial indexes. Restrict this check to cases where the index actually involves spatial attributes (or type is spatial).
- if (!empty($orders) && !$this->spatialIndexOrderSupport) {
+ if ($type === Database::INDEX_SPATIAL && !empty($orders) && !$this->spatialIndexOrderSupport) {
$this->message = 'Spatial indexes with explicit orders are not supported. Remove the orders to create this index.';
return false;
}Optionally, track a $hasSpatial flag in the loop and use if ($hasSpatial && !empty($orders) && !$this->spatialIndexOrderSupport) to be even more precise.
📝 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 (!empty($orders) && !$this->spatialIndexOrderSupport) { | |
| $this->message = 'Spatial indexes with explicit orders are not supported. Remove the orders to create this index.'; | |
| return false; | |
| } | |
| if ($type === Database::INDEX_SPATIAL && !empty($orders) && !$this->spatialIndexOrderSupport) { | |
| $this->message = 'Spatial indexes with explicit orders are not supported. Remove the orders to create this index.'; | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/Database/Validator/Index.php around lines 374 to 377, the current check
rejects any index with explicit orders when the adapter lacks
spatial-index-order support, but it should only apply to indexes that include
spatial attributes or are of spatial type; update the logic to detect whether
the index contains spatial attributes (e.g., set a $hasSpatial flag during the
attribute loop or inspect the index type), then change the condition to run only
when $hasSpatial is true (for example: if ($hasSpatial && !empty($orders) &&
!$this->spatialIndexOrderSupport) { ... }), so non-spatial indexes with orders
remain allowed.
| return self::TYPE_ARRAY; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align getType() with accepted inputs and isArray()
This validator accepts arrays and WKT strings, but getType() returns TYPE_ARRAY while isArray() returns false. This can desync schema/type expectations and lead to subtle validation/UI issues. Either:
- return a mixed/union-friendly type (if available), or
- set isArray() to true and reject strings, or
- keep a custom literal type (previous behavior) that doesn’t imply array-only.
Run to see how getType()/isArray() are used across validators:
🏁 Script executed:
#!/bin/bash
rg -nP 'class\s+\w+\s+extends\s+Validator|function\s+getType|function\s+isArray' -C2 --type=phpLength of output: 10467
Align getType() with accepted inputs and isArray()
File: src/Database/Validator/Spatial.php — getType() returns self::TYPE_ARRAY while isArray() returns false; the same mismatch appears in other validators (Permissions, Structure, Roles, Authorization, …). This desynchronization can break schema/type expectations and UI/validation. Choose and apply one consistent fix across validators: (a) return a non-array type (e.g. TYPE_STRING or a union) that matches accepted WKT strings, (b) set isArray() to true and disallow strings, or (c) keep a distinct literal type but document the behavior.
🤖 Prompt for AI Agents
In src/Database/Validator/Spatial.php around lines 144-146, getType() currently
returns self::TYPE_ARRAY while isArray() returns false, creating a mismatch;
update getType() to return a non-array type that matches the validator's
accepted WKT string inputs (e.g., self::TYPE_STRING or an appropriate union
type) so the type and isArray() are consistent, then apply the same change to
the other validators mentioned (Permissions, Structure, Roles, Authorization,
etc.) to keep behavior uniform across the codebase and ensure
schema/UI/validation expectations are preserved.
updated array conditon for filter
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/Database.php (2)
2013-2047: Bug: fall-through throws “Unknown attribute type” for valid spatial defaults.In validateDefaultTypes(), spatial cases fall into default and always throw. Add a break.
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 - default: + 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);
2310-2315: Enforce required spatial filters or auto-add during updates.You validate required filters here, but getRequiredFilters() doesn’t include spatial types. Without the spatial filter, arrays won’t be encoded to WKT anymore.
- Option A (strict): update getRequiredFilters() to return [$type] for spatial types so updates fail if the filter is removed.
- Option B (DX): when $type is spatial, auto-append $type to $filters if missing, mirroring createAttribute.
Proposed getRequiredFilters():
protected function getRequiredFilters(?string $type): array { return match ($type) { self::VAR_DATETIME => ['datetime'], self::VAR_POINT, self::VAR_LINESTRING, self::VAR_POLYGON => [$type], default => [], }; }
♻️ Duplicate comments (5)
src/Database/Database.php (5)
2429-2429: Adapter call order is correct; ensure Mirror forwards $required as last arg.Database calls
updateAttribute(..., $newKey, $required)which matches adapters. Verify Mirror/Pool/middle layers forward in the same order.#!/bin/bash rg -nP '->updateAttribute\s*\(' -C1 src/Database | sed -n '1,200p'
509-534: Spatial linestring filter: align decode contract and input guard.
- Decode should accept mixed and early-return non-strings.
- Docblock currently misleads; returns array.
self::addFilter( Database::VAR_LINESTRING, /** * @param mixed $value * @return mixed */ function (mixed $value) { if (!is_array($value)) { return $value; } try { - return self::encodeSpatialData($value, Database::VAR_LINESTRING); + return self::encodeSpatialData($value, Database::VAR_LINESTRING); } catch (\Throwable) { return $value; } }, /** - * @param string|null $value - * @return string|null + * @param mixed $value + * @return mixed */ - function (?string $value) { - if (is_null($value)) { - return $value; - } - return self::decodeSpatialData($value); + function (mixed $value) { + if (!\is_string($value)) { + return $value; + } + return self::decodeSpatialData($value); } );
1696-1699: Auto-append spatial filter on createAttribute — also normalize indexes and mirror in createAttributes().
- Use array_values after array_unique (matches createCollection).
- Replicate the same auto-append in createAttributes() to avoid drift when bulk-creating.
if (in_array($type, Database::SPATIAL_TYPES)) { - $filters[] = $type; - $filters = array_unique($filters); + $filters[] = $type; + $filters = \array_values(\array_unique($filters)); }If you want, I can patch createAttributes() similarly by appending
$attribute['type']to$attribute['filters']when spatial, with array_values/unique.
481-507: Spatial point filter: fix decode signature/guard and trim double-space.
- Decode returns arrays but docblock/types say string|null.
- Guard should only decode strings; otherwise return value as-is.
- Remove extra space before
self::encodeSpatialData.self::addFilter( Database::VAR_POINT, /** - * @param mixed $value - * @return mixed + * @param mixed $value + * @return mixed */ function (mixed $value) { if (!is_array($value)) { return $value; } try { - return self::encodeSpatialData($value, Database::VAR_POINT); + return self::encodeSpatialData($value, Database::VAR_POINT); } catch (\Throwable) { return $value; } }, /** - * @param string|null $value - * @return string|null + * @param mixed $value + * @return mixed */ - function (?string $value) { - if (!is_string($value)) { - return $value; - } - return self::decodeSpatialData($value); + function (mixed $value) { + if (!\is_string($value)) { + return $value; + } + return self::decodeSpatialData($value); } );
536-561: Spatial polygon filter: align decode contract and input guard.
- Same issues as linestring; also remove extra space before
encodeSpatialData.self::addFilter( Database::VAR_POLYGON, /** * @param mixed $value * @return mixed */ function (mixed $value) { if (!is_array($value)) { return $value; } try { - return self::encodeSpatialData($value, Database::VAR_POLYGON); + return self::encodeSpatialData($value, Database::VAR_POLYGON); } catch (\Throwable) { return $value; } }, /** - * @param string|null $value - * @return string|null + * @param mixed $value + * @return mixed */ - function (?string $value) { - if (is_null($value)) { - return $value; - } - return self::decodeSpatialData($value); + function (mixed $value) { + if (!\is_string($value)) { + return $value; + } + return self::decodeSpatialData($value); } );
🧹 Nitpick comments (2)
src/Database/Database.php (2)
2263-2267: Only mark altering for spatial NULL/NOT NULL when “required” actually changes.Current logic forces an ALTER for any spatial attribute on unsupported adapters, even if $required stays the same.
- if (!$this->adapter->getSupportForSpatialIndexNull() && in_array($type, Database::SPATIAL_TYPES)) { - $altering = true; - } + if ( + !$this->adapter->getSupportForSpatialIndexNull() + && \in_array($type, Database::SPATIAL_TYPES, true) + && $required !== (bool)$attribute->getAttribute('required', false) + ) { + $altering = true; + }
7277-7327: Nit: WKT polygon doc comment example uses comma inside coordinates.WKT uses space between ordinates, e.g.,
POLYGON((x1 y1, x2 y2)).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (7)
src/Database/Adapter.php (2)
getSupportForSpatialIndexNull(1065-1065)updateAttribute(585-585)src/Database/Adapter/Pool.php (2)
getSupportForSpatialIndexNull(433-436)updateAttribute(178-181)src/Database/Adapter/MariaDB.php (2)
getSupportForSpatialIndexNull(1883-1886)updateAttribute(417-438)src/Database/Adapter/SQLite.php (2)
getSupportForSpatialIndexNull(976-979)updateAttribute(335-342)src/Database/Adapter/Postgres.php (2)
getSupportForSpatialIndexNull(1961-1964)updateAttribute(543-589)src/Database/Adapter/SQL.php (1)
getSupportForSpatialIndexNull(1501-1504)src/Database/Mirror.php (1)
updateAttribute(393-444)
⏰ 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/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (2)
src/Database/Database.php (2)
1327-1339: Nice: auto-inject spatial filters on createCollection.Ensures spatial encode/decode runs without manual setup. Looks good.
3324-3330: Good: spatial index validation uses IndexException and enforces type sanity.Clear errors for missing/invalid attributes improve UX.
Summary by CodeRabbit
New Features
Bug Fixes
Tests