Spatial attribute update attribute and default fix #675
Conversation
|
Warning Rate limit exceeded@ArnabChatterjee20k has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis change tightens spatial attribute validation and default handling in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Database
participant Adapter
participant Indexes as CollectionIndexes
rect rgba(200,220,255,0.18)
note over Client,Database: Create/Update spatial attribute
Client->>Database: createAttribute/updateAttribute(spatial)
Database->>Adapter: checkSpatialSupport()
Adapter-->>Database: supported / not supported
alt spatial not supported
Database-->>Client: AttributeException
else supported
Database->>Database: validate size==empty && array==empty
Database->>Database: validateDefaultTypes (spatial default = single array)
Database-->>Client: attribute created/updated
end
end
rect rgba(255,230,200,0.18)
note over Database,Indexes: Post-update spatial index nullability check
Database->>Adapter: supportsSpatialIndexNull?
Adapter-->>Database: true / false
alt false
Database->>Indexes: scan for spatial indexes
Database->>Database: ensure indexed spatial attrs are required
alt any not required
Database-->>Client: IndexException (spatial index cannot be null)
else
Database-->>Client: OK
end
else true
Database-->>Client: OK
end
end
sequenceDiagram
autonumber
participant Client
participant Database
note over Client,Database: Default validation on attribute creation
Client->>Database: createAttribute(type, default)
alt spatial type (point/linestring/polygon)
Database->>Database: accept default as single array value (no per-element recursion)
Database-->>Client: success or validation error
else non-spatial
Database->>Database: recursively validate array elements (if array)
Database-->>Client: success or validation error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/Database/Database.php (1)
1912-1917: Default validation: correct special-casing for spatial arrays.Treating spatial defaults as a single array (no element-wise recursion) is right. Minor readability tweak below to avoid a misleading “no break” fallthrough.
Apply this diff to make intent explicit in the switch that follows:
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;tests/e2e/Adapter/Scopes/SpatialTests.php (6)
342-349: Remove redundant branch and use assertTrue for clarity.Both branches call the same createIndex; collapse to a single call.
Apply this diff:
- if ($database->getAdapter()->getSupportForSpatialIndexNull()) { - $this->assertEquals(true, $database->createIndex($collectionName, 'idx_line', Database::INDEX_SPATIAL, ['lineAttr'])); - } else { - // Attribute was created as required above; directly create index once - $this->assertEquals(true, $database->createIndex($collectionName, 'idx_line', Database::INDEX_SPATIAL, ['lineAttr'])); - } + $this->assertTrue($database->createIndex($collectionName, 'idx_line', Database::INDEX_SPATIAL, ['lineAttr']));
1791-1796: Make collection name unique to avoid rare cross-test collisions.Minor hardening; several tests already use fixed names, but uniqueness removes flakiness if cleanup ever fails.
- $collectionName = 'spatial_update_attrs_'; + $collectionName = 'spatial_update_attrs_' . uniqid();
1814-1827: DRY up repeated try/catch with a tiny helper closure.Reduces repetition and keeps assertions focused.
You can inline this helper in the test method and reuse it:
$assertThrows = function (callable $fn): void { try { $fn(); $this->fail('Expected exception'); } catch (\Throwable $e) { $this->assertInstanceOf(Exception::class, $e); } }; $assertThrows(fn() => $database->updateAttribute($collectionName, 'geom', size: 10)); $assertThrows(fn() => $database->updateAttribute($collectionName, 'geom', array: true));
1832-1848: Don’t assume attribute order; assert on the specific attribute by key.Index 0 is fragile. Look up the 'geom' attribute before asserting required.
- $database->updateAttribute($collectionName, 'geom', required: false); - $meta = $database->getCollection($collectionName); - $this->assertEquals(false, $meta->getAttribute('attributes')[0]['required']); + $database->updateAttribute($collectionName, 'geom', required: false); + $meta = $database->getCollection($collectionName); + $attrs = $meta->getAttribute('attributes', []); + $geomAttr = null; + foreach ($attrs as $attr) { + if (($attr['key'] ?? $attr['$id']) === 'geom') { $geomAttr = $attr; break; } + } + $this->assertNotNull($geomAttr, 'geom attribute not found'); + $this->assertFalse($geomAttr['required']);Also assert the exception type on the non-nullable-adapter branch:
- try { - $database->updateAttribute($collectionName, 'geom', required: false); - } catch (\Throwable $e) { - $threw = true; - } + try { + $database->updateAttribute($collectionName, 'geom', required: false); + } catch (\Throwable $e) { + $threw = true; + $this->assertInstanceOf(Exception::class, $e); + }
1882-1891: Prefer assertTrue over assertEquals(true, …).Improves intent/readability on boolean-returning APIs.
- $this->assertEquals(true, $database->createAttribute($collectionName, 'pt', Database::VAR_POINT, 0, false, [1.0, 2.0])); - $this->assertEquals(true, $database->createAttribute($collectionName, 'ln', Database::VAR_LINESTRING, 0, false, [[0.0, 0.0], [1.0, 1.0]])); - $this->assertEquals(true, $database->createAttribute($collectionName, 'pg', Database::VAR_POLYGON, 0, false, [[[0.0, 0.0], [0.0, 2.0], [2.0, 2.0], [0.0, 0.0]]])); + $this->assertTrue($database->createAttribute($collectionName, 'pt', Database::VAR_POINT, 0, false, [1.0, 2.0])); + $this->assertTrue($database->createAttribute($collectionName, 'ln', Database::VAR_LINESTRING, 0, false, [[0.0, 0.0], [1.0, 1.0]])); + $this->assertTrue($database->createAttribute($collectionName, 'pg', Database::VAR_POLYGON, 0, false, [[[0.0, 0.0], [0.0, 2.0], [2.0, 2.0], [0.0, 0.0]]]));
1949-1967: Strengthen invalid-default assertions by checking exception type.Avoids false positives from unrelated throwables.
- } catch (\Throwable $e) { - $this->assertTrue(true); + } catch (\Throwable $e) { + $this->assertInstanceOf(Exception::class, $e); } - try { + try { $database->updateAttributeDefault($collectionName, 'ln', [1.0, 2.0]); // wrong dimensionality $this->fail('Expected exception for invalid linestring default shape'); - } catch (\Throwable $e) { - $this->assertTrue(true); + } catch (\Throwable $e) { + $this->assertInstanceOf(Exception::class, $e); } - try { + try { $database->updateAttributeDefault($collectionName, 'pg', [[1.0, 2.0]]); // wrong dimensionality $this->fail('Expected exception for invalid polygon default shape'); - } catch (\Throwable $e) { - $this->assertTrue(true); + } catch (\Throwable $e) { + $this->assertInstanceOf(Exception::class, $e); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Database/Database.php(4 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Database/Database.php (7)
src/Database/Adapter.php (2)
getSupportForSpatialAttributes(1036-1036)getSupportForSpatialIndexNull(1043-1043)src/Database/Adapter/Pool.php (2)
getSupportForSpatialAttributes(428-431)getSupportForSpatialIndexNull(433-436)src/Database/Adapter/MariaDB.php (2)
getSupportForSpatialAttributes(2157-2160)getSupportForSpatialIndexNull(2167-2170)src/Database/Adapter/SQL.php (2)
getSupportForSpatialAttributes(1496-1499)getSupportForSpatialIndexNull(1506-1509)src/Database/Adapter/SQLite.php (2)
getSupportForSpatialAttributes(970-973)getSupportForSpatialIndexNull(975-978)src/Database/Adapter/Postgres.php (2)
getSupportForSpatialAttributes(2261-2264)getSupportForSpatialIndexNull(2271-2274)src/Database/Document.php (1)
getAttribute(216-223)
tests/e2e/Adapter/Scopes/SpatialTests.php (3)
src/Database/Database.php (6)
Database(37-7180)getAdapter(1111-1114)createIndex(3118-3273)updateAttribute(2139-2323)getCollection(1415-1431)updateAttributeDefault(2108-2119)src/Database/Exception.php (1)
Exception(7-21)src/Database/Adapter.php (5)
getSupportForSpatialIndexNull(1043-1043)createIndex(649-649)getSupportForSpatialAttributes(1036-1036)updateAttribute(563-563)getSupportForSpatialIndexOrder(1050-1050)
⏰ 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 (5)
src/Database/Database.php (2)
1855-1860: Spatial attrs: enforcing size=0 and non-array is correct.The new checks align spatial attributes with engine capabilities and mirror other simple types’ constraints. LGTM.
2186-2199: Update path parity: spatial type checks mirror create path.Re-validating adapter support, size=0, and non-array on update prevents drift. Looks good.
tests/e2e/Adapter/Scopes/SpatialTests.php (3)
7-7: Importing base Database exception is correct.This enables explicit assertion of thrown exception types in the new tests.
1795-1808: Good negative coverage for spatial create() constraints.Asserting exceptions for non-zero size and array=true on spatial types matches the tightened validations.
1851-1863: Order-support check looks solid and includes cleanup.Covers both supported and unsupported adapters correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Database/Database.php (2)
2247-2274: Spatial-index nullability enforcement is now correct and scoped — nice fixChecks each indexed spatial attribute’s own required flag, filters to spatial indexes, and guards missing keys. This addresses the earlier review concern.
2036-2041: Prevent toggling required=false on spatial attrs in spatial indexes when NULLs unsupportedupdateAttributeRequired bypasses the new index nullability guard. Disallow making an indexed spatial attribute nullable when the adapter can’t handle NULLs.
- public function updateAttributeRequired(string $collection, string $id, bool $required): Document - { - return $this->updateAttributeMeta($collection, $id, function ($attribute) use ($required) { - $attribute->setAttribute('required', $required); - }); - } + public function updateAttributeRequired(string $collection, string $id, bool $required): Document + { + return $this->updateAttributeMeta($collection, $id, function ($attribute, $collectionDoc) use ($required) { + // Enforce: spatial indexes cannot include nullable spatial attributes on engines without NULL support + if ( + $required === false + && in_array($attribute->getAttribute('type'), self::SPATIAL_TYPES, true) + && !$this->adapter->getSupportForSpatialIndexNull() + ) { + $indexes = $collectionDoc->getAttribute('indexes', []); + foreach ($indexes as $index) { + if ( + $index->getAttribute('type') === self::INDEX_SPATIAL + && in_array($attribute->getAttribute('key'), $index->getAttribute('attributes', []), true) + ) { + throw new IndexException('Spatial indexes do not allow null values. Mark the attribute "' . $attribute->getAttribute('key') . '" as required or drop the spatial index first.'); + } + } + } + $attribute->setAttribute('required', $required); + }); + }
🧹 Nitpick comments (3)
src/Database/Database.php (3)
2200-2201: Error message should list spatial types tooDefault branch now excludes spatial types from the “Must be one of …” list despite supporting them above.
- 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); + 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);
2224-2234: Minor: method name casingUse setAttribute (current casing is setattribute). PHP is case-insensitive here, but consistency helps static analysis and readability.
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
1912-1914: Consistency: prefer self::SPATIAL_TYPES inside the classTiny style nit: use self::SPATIAL_TYPES (as done elsewhere) instead of Database::SPATIAL_TYPES.
- if (!in_array($type, Database::SPATIAL_TYPES)) { + if (!in_array($type, self::SPATIAL_TYPES, true)) {
📜 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(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Database.php (7)
src/Database/Adapter.php (2)
getSupportForSpatialAttributes(1036-1036)getSupportForSpatialIndexNull(1043-1043)src/Database/Adapter/MariaDB.php (2)
getSupportForSpatialAttributes(2157-2160)getSupportForSpatialIndexNull(2167-2170)src/Database/Adapter/Pool.php (2)
getSupportForSpatialAttributes(428-431)getSupportForSpatialIndexNull(433-436)src/Database/Adapter/Postgres.php (2)
getSupportForSpatialAttributes(2261-2264)getSupportForSpatialIndexNull(2271-2274)src/Database/Adapter/SQL.php (2)
getSupportForSpatialAttributes(1496-1499)getSupportForSpatialIndexNull(1506-1509)src/Database/Adapter/SQLite.php (2)
getSupportForSpatialAttributes(970-973)getSupportForSpatialIndexNull(975-978)src/Database/Document.php (1)
getAttribute(216-223)
⏰ 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/Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (2)
src/Database/Database.php (2)
1855-1860: Spatial attribute creation: size/array guards look goodCorrectly enforces size=0 and array=false for spatial attributes.
2186-2199: Spatial attribute update: parity with creation checks — goodMirrors create-time validation (support check, size=0, array=false). Looks consistent.
Co-authored-by: Jake Barnby <jakeb994@gmail.com>
Co-authored-by: Jake Barnby <jakeb994@gmail.com>
Co-authored-by: Jake Barnby <jakeb994@gmail.com>
2 participants
Summary by CodeRabbit