added index validtor for spatial index #674
Conversation
WalkthroughAdds three spatial index toggles to the Index validator, extends its constructor to accept them, invokes a new checkSpatialIndex during validation, and implements checkSpatialIndex(Document): bool to enforce spatial type usage, nullability, and ordering constraints, setting an error message on violation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant DB as Database
participant IndexValidator as Index Validator
participant AttrMap as Attributes Map
Caller->>DB: createIndex/updateAttribute/createCollection(...)
DB->>IndexValidator: __construct(..., arrayIndexSupport, spatialIndexSupport, spatialIndexNullSupport, spatialIndexOrderSupport)
DB->>IndexValidator: isValid(index)
activate IndexValidator
IndexValidator->>IndexValidator: run existing checks (...)
IndexValidator->>IndexValidator: checkSpatialIndex(index)
activate AttrMap
IndexValidator->>AttrMap: resolve attribute type & nullability
AttrMap-->>IndexValidator: type, required flag
deactivate AttrMap
alt spatial type present and spatialIndexSupport == false
IndexValidator-->>DB: false (message: Spatial index is not supported)
else spatial type present & nullable & spatialIndexNullSupport == false
IndexValidator-->>DB: false (message: Disallow nulls for spatial index / mark required)
else spatial type present & has orders & spatialIndexOrderSupport == false
IndexValidator-->>DB: false (message: Remove explicit orders for spatial index)
else
IndexValidator-->>DB: true
end
deactivate IndexValidator
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Database/Validator/Index.php (1)
27-31: Three new spatial capability flags added — OK, but consider reducing boolean-parameter sprawlThe new properties look fine. However, adding multiple booleans increases constructor complexity and call-site ambiguity. Consider grouping these into a small immutable value object (e.g., Capabilities or AdapterSupport) passed as a single arg to improve readability and future extensibility.
📜 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/Index.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-7129)
⏰ 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 (4)
src/Database/Validator/Index.php (4)
38-40: Docblock updated — LGTMParameters are documented and consistent with the new properties.
48-50: Property wiring — LGTMThe three capability flags are stored as-is. No issues spotted.
304-307: Unconditional spatial check is fine, but the method must short-circuit for non-spatial indexesCalling checkSpatialIndex() for every index is OK, provided the method itself returns early unless $index->getAttribute('type') === Database::INDEX_SPATIAL. Otherwise, you risk blocking KEY/UNIQUE indexes on spatial attributes unnecessarily.
43-43: I’m inspecting the E2E test call sites to confirm how many arguments they pass to the new constructor signature. Once we verify that none mix up the new boolean flags, I’ll update the review comment.
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)
1926-1936: Bug: missing break causes spatial defaults to always throw "Unknown attribute type"In validateDefaultTypes(), the spatial cases fall through into the default branch due to the “no break” comment, causing a false error even when the default is a valid array.
Apply a break after validating spatial defaults:
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);
7045-7050: Validate spatial payloads: throw on invalid input instead of ignoring validator resultencodeSpatialData() calls Spatial->isValid($value) but ignores its boolean result. If validation fails, we still proceed and emit WKT. Fail fast with a clear exception.
Apply:
- $validator = new Spatial($type); - $validator->isValid($value); + $validator = new Spatial($type); + if (!$validator->isValid($value)) { + throw new DatabaseException($validator->getDescription()); + }
🧹 Nitpick comments (6)
src/Database/Database.php (6)
3060-3066: Docblock types for $lengths/$orders should allow nullsBoth arrays can contain nulls after normalization (e.g., arrays or max-length handling). Update PHPDoc to reflect reality to help static analysis.
- * @param array<int> $lengths - * @param array<string> $orders + * @param array<int|null> $lengths + * @param array<string|null> $orders
3161-3182: Avoid duplicating spatial index validation here; let IndexValidator enforce itThis block re-validates attribute existence, spatial types, and nullability, which IndexValidator::checkSpatialIndex is already designed to enforce (now that you pass the adapter flags). Consider keeping only quick adapter feature guards (e.g., orders support) and remove deeper checks to prevent drift.
I can submit a follow-up PR that trims these checks and expands IndexValidator tests to cover all cases (type, nullability, orders) so Database stays thin. Want me to proceed?
3168-3171: Minor: use the central SPATIAL_TYPES constant for consistencyReplace the hard-coded list with Database::SPATIAL_TYPES to keep a single source of truth.
- if (!in_array($attributeType, [self::VAR_POINT, self::VAR_LINESTRING, self::VAR_POLYGON])) { + if (!in_array($attributeType, self::SPATIAL_TYPES, true)) {
2201-2203: Nit: method name casingPHP method names are case-insensitive, but elsewhere in this file you use setAttribute(). Align casing for consistency.
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
60-60: Nit: formatting of SPATIAL_TYPES constantTiny readability tweak: add a space after the first comma.
- public const SPATIAL_TYPES = [self::VAR_POINT,self::VAR_LINESTRING, self::VAR_POLYGON]; + public const SPATIAL_TYPES = [self::VAR_POINT, self::VAR_LINESTRING, self::VAR_POLYGON];
3076-3205: Add targeted tests for spatial index validation pathsGiven the new flags and validation logic, please add integration tests that cover:
- Creating a spatial index when adapter reports no spatial attribute support → expect DatabaseException.
- Spatial index with explicit orders when adapter disallows orders → expect DatabaseException.
- Spatial index on non-spatial attribute → expect IndexException from validator.
- Spatial index on nullable attribute when adapter disallows nulls → expect IndexException.
- Happy paths on adapters that support all three flags.
I can generate a PHPUnit test skeleton that stubs adapter capabilities and asserts the thrown exceptions. Want me to draft it?
📜 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/Database.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (7)
src/Database/Adapter/MariaDB.php (4)
getSupportForIndexArray(2152-2155)getSupportForSpatialAttributes(2157-2160)getSupportForSpatialIndexNull(2167-2170)getSupportForSpatialIndexOrder(2186-2189)src/Database/Adapter.php (4)
getSupportForIndexArray(914-914)getSupportForSpatialAttributes(1036-1036)getSupportForSpatialIndexNull(1043-1043)getSupportForSpatialIndexOrder(1050-1050)src/Database/Adapter/Pool.php (4)
getSupportForIndexArray(338-341)getSupportForSpatialAttributes(428-431)getSupportForSpatialIndexNull(433-436)getSupportForSpatialIndexOrder(529-532)src/Database/Adapter/MySQL.php (2)
getSupportForIndexArray(81-87)getSupportForSpatialIndexOrder(126-129)src/Database/Adapter/SQL.php (4)
getSupportForIndexArray(1466-1469)getSupportForSpatialAttributes(1496-1499)getSupportForSpatialIndexNull(1506-1509)getSupportForSpatialIndexOrder(1516-1519)src/Database/Adapter/Postgres.php (3)
getSupportForSpatialAttributes(2261-2264)getSupportForSpatialIndexNull(2271-2274)getSupportForSpatialIndexOrder(2291-2294)src/Database/Adapter/SQLite.php (3)
getSupportForSpatialAttributes(970-973)getSupportForSpatialIndexNull(975-978)getSupportForSpatialIndexOrder(1258-1261)
⏰ 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 (Pool)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (3)
src/Database/Database.php (3)
2257-2261: Consistent propagation of spatial flags in update flowSame correct ordering of the new flags when validating indexes after attribute update. Good consistency with createCollection.
3200-3204: CreateIndex path also passes spatial flags — consistent with other call sitesAll three call sites (createCollection, updateAttribute, createIndex) now propagate the new adapter capabilities to the validator. Nice.
1309-1313: I've requested the full context around eachnew IndexValidatorinstantiation to confirm they all include the new spatial support flags. Once you provide the outputs, I'll verify the argument counts and confirm whether everything has been updated correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Validator/Index.php (1)
335-376: Spatial index validation scoped to INDEX_SPATIAL with strict type checks — LGTM
- Early return for non-spatial indexes.
- Clear capability gate: spatialIndexSupport.
- Enforces: all attributes spatial, required/null constraint, explicit orders gate.
- Uses strict in_array for Database::SPATIAL_TYPES.
This aligns with prior review guidance.
🧹 Nitpick comments (6)
src/Database/Validator/Index.php (6)
38-41: Constructor grows a “boolean train” — consider an options value-object or associative arraySeven positional params (four booleans) are error-prone. A small options DTO (e.g., IndexCapabilities) or a single associative array for spatial capabilities will improve readability and reduce call-site mistakes. This can be a follow-up; current change remains backward compatible due to defaults.
Also applies to: 43-43
304-307: Run spatial validation earlier in isValid to surface relevant errors firstToday, length/order/array checks can fail before spatial-specific messages. Moving checkSpatialIndex() earlier (e.g., right after duplicated-attributes or after fulltext check) provides clearer, more targeted feedback for INDEX_SPATIAL.
Apply this reordering:
@@ public function isValid($value): bool - if (!$this->checkArrayIndex($value)) { + if (!$this->checkArrayIndex($value)) { return false; } - if (!$this->checkIndexLength($value)) { + // Spatial-specific constraints should be evaluated early + if (!$this->checkSpatialIndex($value)) { + return false; + } + + if (!$this->checkIndexLength($value)) { return false; } - if (!$this->checkReservedNames($value)) { + if (!$this->checkReservedNames($value)) { return false; } - - if (!$this->checkSpatialIndex($value)) { - return false; - }
358-361: Build the allowed-type list from Database::SPATIAL_TYPES to avoid message driftHardcoding “point, linestring, polygon” can desync if new spatial types are added. Construct the message dynamically.
Apply:
- if (!\in_array($attributeType, Database::SPATIAL_TYPES, true)) { - $this->message = 'Spatial index can only be created on spatial attributes (point, linestring, polygon). Attribute "' . $attributeName . '" is of type "' . $attributeType . '"'; + if (!\in_array($attributeType, Database::SPATIAL_TYPES, true)) { + $this->message = 'Spatial index can only be created on spatial attributes (' . \implode(', ', Database::SPATIAL_TYPES) . '). Attribute "' . $attributeName . '" is of type "' . $attributeType . '"'; return false; }
351-353: Optional: validate orders cardinality and values when “order support” is enabledIf orders are provided, it’s useful to ensure they match the attributes count and contain only allowed directions (ASC/DESC), when spatialIndexOrderSupport is true.
Apply:
$attributes = $index->getAttribute('attributes', []); - $orders = $index->getAttribute('orders', []); + $orders = $index->getAttribute('orders', []); @@ - if (!empty($orders) && !$this->spatialIndexOrderSupport) { + 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 (!empty($orders) && $this->spatialIndexOrderSupport) { + if (\count($orders) !== \count($attributes)) { + $this->message = 'Number of orders must match number of attributes for spatial indexes.'; + return false; + } + foreach ($orders as $dir) { + if (!\in_array(\strtoupper((string)$dir), ['ASC', 'DESC'], true)) { + $this->message = 'Invalid order "' . $dir . '" for spatial index. Allowed: ASC or DESC.'; + return false; + } + } + }Also applies to: 370-373
335-376: Add unit tests for spatial-index validation branchesPlease add tests covering:
- Spatial index on non-spatial attribute → fails with “can only be created on spatial attributes”.
- Spatial index with nullable attribute when spatialIndexNullSupport=false → fails with null message.
- Spatial index with explicit orders when spatialIndexOrderSupport=false → fails with orders message.
- Spatial index when spatialIndexSupport=false → fails with “not supported”.
- Non-spatial index that includes a spatial attribute → passes (ensures early return works).
I can draft PHPUnit tests targeting Index::isValid() with appropriately mocked Documents. Want me to open a follow-up PR or attach a patch?
351-353: Reject spatial index prefix lengths for clearer UXA quick grep across all adapters (
rg -nC3 'lengths|INDEX_SPATIAL' src/Database --type=php) shows no driver actually supports prefix lengths on spatial indexes. To surface this sooner with a clear error message (instead of cryptic SQL failures), add a check in the validator.• File:
src/Database/Validator/Index.php
• Location:checkSpatialIndex(Document $index)(top of the method)Suggested diff:
public function checkSpatialIndex(Document $index): bool { + // Disallow prefix lengths on spatial indexes + $lengths = $index->getAttribute('lengths', []); + if (!empty($lengths)) { + $this->message = 'Spatial indexes do not support index prefix lengths.'; + return false; + } $type = $index->getAttribute('type'); if ($type !== Database::INDEX_SPATIAL) { return true; } … }This will catch invalid
lengthsup front with a helpful message.
📜 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/Index.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Validator/Index.php (1)
src/Database/Database.php (1)
Database(37-7138)
⏰ 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 (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (Postgres)
🔇 Additional comments (1)
src/Database/Validator/Index.php (1)
27-31: Spatial capability flags added — LGTMClear, typed booleans with safe default semantics. No concerns here.
2 participants
added index validtor for spatial index
Summary by CodeRabbit