Spatial decoding #698
Spatial decoding #698
Conversation
…code-decode # Conflicts: # src/Database/Database.php
|
Warning Rate limit exceeded@fogelito has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 55 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 (2)
WalkthroughSpatial decoding was moved from Database to adapters: Database delegates spatial decode operations to adapter methods. Adapter API gained decodePoint/decodeLinestring/decodePolygon and tightened getAttributeProjection to return string. SQL/Postgres/Pool implement or delegate new decoders; tests add spatial fixtures and assertions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Database
participant Pool as AdapterPool
participant Impl as AdapterImpl (SQL/Postgres)
Client->>Database: getDocument/createDocument(...)
Database->>Pool: fetch rows (spatial columns as WKB/WKT)
alt spatial columns present
Database->>Pool: decodePoint/Linestring/Polygon(wkb|wkt)
Pool->>Impl: delegate decode*(...)
Impl-->>Pool: geometry arrays
Pool-->>Database: geometry arrays
Database-->>Client: document with decoded spatial fields
else no spatial
Database-->>Client: document (no decode calls)
end
sequenceDiagram
autonumber
participant Database
participant SQL as SQL Adapter
Database->>SQL: getDocument(selections, alias)
note over SQL: Projection generation
SQL->>SQL: getAttributeProjection(selections, alias)
SQL-->>Database: rows (spatial columns still WKB/WKT)
Database->>SQL: decode*(...) per spatial field
SQL-->>Database: decoded coordinate arrays
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Database/Database.php (3)
2179-2184: Prevent removal of required filters in updateAttributeFilters.This path can strip mandatory filters from spatial/datetime attributes, breaking encoding/decoding. Validate here too.
Apply:
public function updateAttributeFilters(string $collection, string $id, array $filters): Document { - return $this->updateAttributeMeta($collection, $id, function ($attribute) use ($filters) { - $attribute->setAttribute('filters', $filters); - }); + return $this->updateAttributeMeta($collection, $id, function ($attribute) use ($filters) { + $type = $attribute->getAttribute('type'); + $required = $this->getRequiredFilters($type); + if (!empty(array_diff($required, $filters))) { + throw new DatabaseException("Attribute of type: {$type} requires the following filters: " . implode(",", $required)); + } + $attribute->setAttribute('filters', $filters); + }); }
2023-2033: Bug: spatial default validation always throws “Unknown attribute type”.Spatial cases fall through to default without a break, so even valid arrays raise an exception.
Fix:
- 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: + 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'); + } + 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);
1970-1979: updateAttributeFilters bypasses required-filter validation — fix in Database.phpsrc/Database/Database.php:2179 (updateAttributeFilters) sets filters directly via updateAttributeMeta without checking $this->getRequiredFilters($type); src/Database/Mirror.php:961 delegates to it. Enforce the same required-filter check used in updateAttribute/createAttribute (compute requiredFilters from the attribute's type and throw DatabaseException if any are missing) or route this call through updateAttribute to reuse existing validation.
🧹 Nitpick comments (3)
src/Database/Database.php (3)
528-534: Harden linestring decode against non-string input (parity with point).Guard early if $value isn’t a string to avoid TypeError/mis-decoding.
- function (?string $value) { - if (is_null($value)) { - return $value; - } - return self::decodeSpatialData($value); - } + function (?string $value) { + if (!is_string($value)) { + return $value; + } + return self::decodeSpatialData($value); + }
555-561: Harden polygon decode against non-string input (parity with point).Same early-return guard as other decoders.
- function (?string $value) { - if (is_null($value)) { - return $value; - } - return self::decodeSpatialData($value); - } + function (?string $value) { + if (!is_string($value)) { + return $value; + } + return self::decodeSpatialData($value); + }
498-506: Docblock return types don’t match actual decoder returns.Decode callbacks for spatial types return arrays, but annotations say string|null. Adjust to reduce confusion for static analyzers.
- * @param string|null $value - * @return string|null + * @param string|null $value + * @return array<mixed>|string|nullAlso applies to: 525-533, 552-560
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
phpunit.xml(1 hunks)src/Database/Database.php(1 hunks)tests/e2e/Adapter/Base.php(1 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/SpatialTests.php (3)
src/Database/Database.php (2)
createAttribute(1676-1727)Database(37-7364)src/Database/Adapter/Pool.php (2)
createAttribute(168-171)getSupportForSpatialIndexNull(433-436)src/Database/Adapter.php (2)
createAttribute(558-558)getSupportForSpatialIndexNull(1065-1065)
🪛 GitHub Actions: Linter
tests/e2e/Adapter/Base.php
[error] 1-1: Pint lint: PSR-12 statement_indentation violation detected in this file.
⏰ 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 (20)
phpunit.xml (1)
10-10: Fail-fast tests: confirm this is desired for CI.stopOnFailure="true" speeds up feedback but hides downstream failures. If the goal is faster local runs only, consider gating via an env variable or a separate local phpunit.xml.dist.
tests/e2e/Adapter/Scopes/SpatialTests.php (18)
80-80: LGTM! The explicit filter specification aligns with the PR's approach to enforce spatial type filters.Based on the relevant code snippets from
src/Database/Database.php, the PR has updated the required filter validation to enforce spatial attributes must include their specific type filters (point, linestring, polygon). This change ensures proper spatial type validation without relying on auto-injection.
108-110: LGTM! Consistent application of required spatial filters.The explicit filter specification across all spatial types (point, linestring, polygon) properly addresses the PR's validation changes. Each
createAttributecall now correctly includes the required filters parameter.
237-237: LGTM! Consistent spatial filter specification in relationship tests.The explicit
filters: ['point']parameter correctly follows the new requirement for spatial attributes to specify their type filters.
342-344: LGTM! Proper implementation of required spatial filters.All spatial attribute types are correctly specified with their corresponding filters, maintaining consistency with the PR's validation changes.
393-393: LGTM! Consistent spatial attribute definition.The point attribute correctly includes the required
filters: ['point']parameter, following the established pattern throughout the PR.
505-505: LGTM! Consistent spatial filter application.The spatial attribute definition properly includes the required filter specification.
609-612: LGTM! Comprehensive spatial filter implementation.Both point and polygon attributes correctly specify their respective filters, demonstrating proper adherence to the new spatial validation requirements.
711-711: LGTM! Consistent spatial attribute handling.The spatial attribute creation properly includes the required filter specification.
772-772: LGTM! Proper spatial filter specification.The attribute creation correctly includes the required
filters: ['point']parameter.
832-832: LGTM! Consistent application of spatial filters.The spatial attribute creation properly specifies the required filter parameter.
908-914: LGTM! Comprehensive spatial filter implementation.All spatial attribute types (polygon, polygon, polygon, point, polygon, linestring) are correctly defined with their respective required filters, demonstrating thorough compliance with the PR's validation changes.
1338-1341: LGTM! Complete spatial filter specification.All three spatial attribute types are properly defined with their corresponding required filters, ensuring compliance with the new validation requirements.
1769-1770: LGTM! Proper spatial filter implementation.Both spatial attributes correctly specify their required filters in the aggregation test context.
1858-1858: LGTM! Consistent spatial attribute handling.The spatial attribute creation properly includes the required filter specification for validation testing.
1865-1865: LGTM! Proper error case testing with filters.The test correctly includes the required filter even in the error case scenario, ensuring consistent validation behavior.
1872-1872: LGTM! Consistent spatial filter specification.The spatial attribute creation properly includes the required filter parameter for the main test case.
1943-1945: LGTM! Comprehensive spatial defaults testing with filters.All three spatial attribute types correctly specify their required filters while testing default value functionality, ensuring proper validation coverage.
2150-2150: LGTM! Proper spatial filter specification in distance testing.The spatial attribute creation correctly includes the required filter parameter for distance measurement functionality.
tests/e2e/Adapter/Base.php (1)
21-26: Confirm intent: commented-out trait suites in tests/e2e/Adapter/Base.phpCommented trait uses (tests/e2e/Adapter/Base.php, lines 21–26) disable CollectionTests, DocumentTests, AttributeTests, IndexTests, PermissionTests, RelationshipTests; the corresponding scope files still exist at tests/e2e/Adapter/Scopes/{Attribute,Collection,Document,Index,Permission,Relationship}Tests.php — confirm whether this coverage reduction is intentional/temporary for the spatial encode/decode work or a permanent test-strategy change.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Database/Adapter/MariaDB.php (1)
1-1: Remove debug var_dump calls and run the formatter (Pint).rg found debug dumps — remove/replace these and then run Pint to fix PSR-12 "statement_indentation" failures.
- Locations to remove/replace:
- src/Database/Adapter/Postgres.php: lines 996, 2052, 2114
- src/Database/Adapter/SQL.php: lines 369, 2757
- src/Database/Adapter/MariaDB.php: line 867
vendor/bin/pint is missing; install dev deps (e.g., composer install or composer require --dev laravel/pint) and run: vendor/bin/pint -v. Re-run: rg -n "var_dump(" -S to confirm no debug dumps remain.
src/Database/Database.php (3)
514-523: Linestring encode silently passes invalid input; align with point behaviorReturning the original value on invalid input lets bad data slip through. Match point’s strictness: null passthrough, otherwise encode or throw.
Apply:
- function (mixed $value) { - if (!is_array($value)) { - return $value; - } - try { - return self::encodeSpatialData($value, Database::VAR_LINESTRING); - } catch (\Throwable) { - return $value; - } - }, + function (mixed $value) { + if ($value === null) { + return null; + } + try { + return self::encodeSpatialData($value, Database::VAR_LINESTRING); + } catch (\Throwable $e) { + throw new StructureException('Invalid linestring'); + } + },
541-550: Polygon encode also returns invalid input; enforce strictnessSame issue as linestring: invalid inputs should raise, not pass through.
Apply:
- function (mixed $value) { - if (!is_array($value)) { - return $value; - } - try { - return self::encodeSpatialData($value, Database::VAR_POLYGON); - } catch (\Throwable) { - return $value; - } - }, + function (mixed $value) { + if ($value === null) { + return null; + } + try { + return self::encodeSpatialData($value, Database::VAR_POLYGON); + } catch (\Throwable $e) { + throw new StructureException('Invalid polygon'); + } + },
2251-2260: Updating an attribute to spatial type doesn’t enforce adding the spatial filterWhen changing type to point/linestring/polygon, filters may not include the corresponding spatial filter, breaking encode/decode. Ensure filters are updated automatically.
Apply:
$formatOptions ??= $attribute->getAttribute('formatOptions'); - $filters ??= $attribute->getAttribute('filters'); + $filters ??= $attribute->getAttribute('filters'); + // Normalize filters for spatial/non-spatial types + if (\in_array($type, self::SPATIAL_TYPES, true)) { + $filters = \array_values(\array_unique([...$filters, $type])); + } else { + $filters = \array_values(\array_diff($filters, self::SPATIAL_TYPES)); + }
🧹 Nitpick comments (8)
tests/e2e/Adapter/Scopes/SpatialTests.php (1)
123-139: Round‑trip assertions look good; consider delta when asserting floats.Spatial decoders may introduce tiny FP rounding. If you see flakes, switch these to use a small delta.
Example:
- $this->assertEquals($point, $createdDoc->getAttribute('pointAttr')); + $this->assertEqualsWithDelta($point[0], $createdDoc->getAttribute('pointAttr')[0], 1.0E-9); + $this->assertEqualsWithDelta($point[1], $createdDoc->getAttribute('pointAttr')[1], 1.0E-9);src/Database/Adapter/Postgres.php (1)
2129-2129: Remove unused variableThe variable
$isLEis assigned but never used.Remove the unused variable declaration that was pointed out by PHPMD.
src/Database/Database.php (6)
485-493: Point encode: preserve validator details and remove dead codeCatching and replacing the validator’s message with a generic "Invalid point" discards actionable details. Also, the commented adapter call should be removed.
Apply:
- try { - return self::encodeSpatialData($value, Database::VAR_POINT); - //return $this->adapter->encodePoint($value); - } catch (\Throwable) { - throw new StructureException('Invalid point'); - } + try { + return self::encodeSpatialData($value, Database::VAR_POINT); + } catch (\Throwable $e) { + throw new StructureException($e->getMessage(), previous: $e); + }
496-509: Remove stray TODO; decode expects string input"todo:validate array point" is misplaced in a decode path that receives string/Blob WKT/WKB and causes noise.
Apply:
- /** - * todo:validate array point - */
461-465: Fix Pint: explicit null returnThe encoder for datetime returns bare "return;". Pint reports simplified_null_return; return null explicitly.
Apply:
- if (is_null($value)) { - return; - } + if (is_null($value)) { + return null; + }
2333-2336: Typo: setattribute → setAttributeMethod name casing is inconsistent and hurts readability.
Apply:
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
525-536: Error message exposureRe-throwing adapter decode messages verbatim can leak engine-specific details. Consider wrapping with a consistent StructureException message and keep the original as previous for logs.
Also applies to: 551-562
482-510: Add adapter-level spatial encoders or standardize error handling
- Issue: encode uses self::encodeSpatialData (WKT) while decode uses $this->adapter->decodePoint, preventing adapters from applying SRID/engine-specific transforms.
- Action: add encodePoint/encodeLinestring/encodePolygon to Adapter and call them here; if you keep the WKT builder, make error handling consistent across point/linestring/polygon.
- Location: src/Database/Database.php lines 482–510 (also applies to 513–565).
- I can draft the Adapter interface + implementations in a follow-up PR.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Postgres.php(2 hunks)src/Database/Adapter/SQL.php(2 hunks)src/Database/Database.php(3 hunks)src/Database/Validator/Spatial.php(1 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Database/Validator/Spatial.php
🧰 Additional context used
🧬 Code graph analysis (5)
src/Database/Adapter.php (2)
src/Database/Adapter/SQL.php (3)
decodePoint(2714-2741)decodeLinestring(2743-2775)decodePolygon(2777-2852)src/Database/Adapter/Postgres.php (3)
decodePoint(2012-2036)decodeLinestring(2038-2094)decodePolygon(2096-2165)
src/Database/Adapter/Postgres.php (2)
src/Database/Adapter.php (3)
decodePoint(1289-1289)decodeLinestring(1290-1290)decodePolygon(1291-1291)src/Database/Adapter/SQL.php (3)
decodePoint(2714-2741)decodeLinestring(2743-2775)decodePolygon(2777-2852)
tests/e2e/Adapter/Scopes/SpatialTests.php (3)
src/Database/Adapter.php (2)
createDocument(702-702)getDocument(692-692)src/Database/Database.php (2)
createDocument(3836-3927)getDocument(3444-3564)src/Database/Adapter/SQL.php (1)
getDocument(351-418)
src/Database/Adapter/SQL.php (2)
src/Database/Adapter.php (6)
getAttributeProjection(1158-1158)quote(468-468)getTenantQuery(1281-1281)decodePoint(1289-1289)decodeLinestring(1290-1290)decodePolygon(1291-1291)src/Database/Adapter/Pool.php (3)
getAttributeProjection(479-482)quote(118-121)getTenantQuery(504-507)
src/Database/Database.php (3)
src/Database/Adapter.php (3)
decodePoint(1289-1289)decodeLinestring(1290-1290)decodePolygon(1291-1291)src/Database/Adapter/SQL.php (3)
decodePoint(2714-2741)decodeLinestring(2743-2775)decodePolygon(2777-2852)src/Database/Adapter/Postgres.php (3)
decodePoint(2012-2036)decodeLinestring(2038-2094)decodePolygon(2096-2165)
🪛 GitHub Actions: CodeQL
src/Database/Adapter.php
[error] 1289-1289: Method Utopia\Database\Adapter::decodePoint() return type has no value type specified in iterable type array.
[error] 1290-1290: Method Utopia\Database\Adapter::decodeLinestring() return type has no value type specified in iterable type array.
[error] 1291-1291: Method Utopia\Database\Adapter::decodePolygon() return type has no value type specified in iterable type array.
src/Database/Adapter/Postgres.php
[error] 2006-2006: Method Utopia\Database\Adapter\Postgres::encodePoint() has parameter $point with no value type specified in iterable type array.
[error] 2012-2012: Method Utopia\Database\Adapter\Postgres::decodePoint() return type has no value type specified in iterable type array.
[error] 2027-2027: Cannot access offset 0 on string|false.
[error] 2028-2028: Cannot access offset 1 on array|false.
[error] 2028-2028: Parameter #1 $string of function substr expects string, string|false given.
[error] 2032-2032: Cannot access offset 1 on array|false.
[error] 2032-2032: Parameter #1 $string of function substr expects string, string|false given.
[error] 2033-2033: Cannot access offset 1 on array|false.
[error] 2033-2033: Parameter #1 $string of function substr expects string, string|false given.
[error] 2038-2038: Method Utopia\Database\Adapter\Postgres::decodeLinestring() return type has no value type specified in iterable type array.
[error] 2070-2070: Cannot access offset 1 on array|false.
[error] 2083-2083: Cannot access offset 1 on array|false.
[error] 2088-2088: Cannot access offset 1 on array|false.
[error] 2089-2089: Cannot access offset 1 on array|false.
[error] 2096-2096: Method Utopia\Database\Adapter\Postgres::decodePolygon() return type has no value type specified in iterable type array.
[error] 2133-2133: Cannot access offset 1 on array|false.
[error] 2147-2147: Cannot access offset 1 on array|false.
[error] 2152-2152: Cannot access offset 1 on array|false.
[error] 2156-2156: Cannot access offset 1 on array|false.
[error] 2157-2157: Cannot access offset 1 on array|false.
src/Database/Adapter/SQL.php
[error] 2714-2714: Method Utopia\Database\Adapter\SQL::decodePoint() return type has no value type specified in iterable type array.
[error] 2740-2740: Cannot access offset 1 on array|false.
[error] 2740-2740: Cannot access offset 2 on array|false.
[error] 2743-2743: Method Utopia\Database\Adapter\SQL::decodeLinestring() return type has no value type specified in iterable type array.
[error] 2763-2763: Cannot access offset 1 on array|false.
[error] 2768-2768: Cannot access offset 1 on array|false.
[error] 2769-2769: Cannot access offset 1 on array|false.
[error] 2777-2777: Method Utopia\Database\Adapter\SQL::decodePolygon() return type has no value type specified in iterable type array.
[error] 2816-2816: Cannot access offset 1 on array|false.
[error] 2830-2830: Cannot access offset 1 on array|false.
[error] 2836-2836: Cannot access offset 1 on array|false.
[error] 2841-2841: Cannot access offset 1 on array|false.
🪛 GitHub Actions: Linter
src/Database/Adapter/MariaDB.php
[error] 1-1: PSR-12: statement_indentation violation reported by Pint.
src/Database/Adapter/Postgres.php
[error] 1-1: PSR-12: no_multiple_statements_per_line; statement_indentation violations reported by Pint.
src/Database/Adapter/SQL.php
[error] 1-1: PSR-12: statement_indentation; binary_operator_space violations reported by Pint.
src/Database/Database.php
[error] 1-1: PSR-12: method_argument_space; simplified_null_return violations reported by Pint.
🪛 PHPMD (2.15.0)
src/Database/Adapter/Postgres.php
2129-2129: Avoid unused local variables such as '$isLE'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
🔇 Additional comments (1)
tests/e2e/Adapter/Scopes/SpatialTests.php (1)
116-119: Good: test data hoisted to locals for reuse.This reduces duplication and clarifies expectations.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Database/Adapter/MariaDB.php (1)
1-1: Remove stray var_dump() calls and run Pint to fix PSR-12 indentation.Detected var_dump() locations:
- src/Database/Adapter/SQL.php:369, 2757
- src/Database/Adapter/Postgres.php:996, 2046, 2108
- src/Database/Adapter/MariaDB.php:867
Remove those debug statements, then run vendor/bin/pint and re-run the grep check for var_dump.
src/Database/Adapter/SQL.php (1)
1-1: Remove debug var_dump() calls and run Pint to fix PSR-12Remove the var_dump() debug prints below and run vendor/bin/pint to auto-fix formatting; re-run the search to confirm none remain.
- src/Database/Adapter/Postgres.php:996 — var_dump($sql);
- src/Database/Adapter/Postgres.php:2046 — var_dump($wkb);
- src/Database/Adapter/Postgres.php:2108 — var_dump($wkb);
- src/Database/Adapter/SQL.php:369 — var_dump($sql);
- src/Database/Adapter/SQL.php:2757 — var_dump($wkb);
- src/Database/Adapter/MariaDB.php:867 — var_dump($sql);
src/Database/Database.php (2)
461-469: Fix Pint failure: simplify the early null-return in datetime encoderPint reports simplified_null_return. Prefer strict compare and returning the variable itself to satisfy the fixer and keep semantics unchanged.
Apply this diff:
- if (is_null($value)) { - return null; - } + if ($value === null) { + return $value; + }
499-507: Critical: static filter closure captures $this; breaks multi-adapter instances + wrong return phpdocThese filters are stored in a static map (self::$filters). Capturing $this binds the first/last Database instance into a static closure, causing wrong adapter usage when multiple Database instances/adapters coexist. Also, phpdoc says “string|null” but decodePoint returns array.
Use the instance passed by decodeAttribute (third argument) instead, and fix phpdoc/ternary for Pint.
Apply this diff:
- /** - * @param string|null $value - * @return string|null - */ - function (?string $value) { - if ($value === null) { - return null; - } - return $this->adapter->decodePoint($value); - } + /** + * @param string|null $value + * @param Document $_ + * @param Database $db + * @return array|null + */ + function (?string $value, Document $_, Database $db) { + return $value === null ? null : $db->adapter->decodePoint($value); + }
♻️ Duplicate comments (12)
src/Database/Adapter/SQL.php (5)
362-362: Do not override computed spatial attributesThis debug assignment disables ST_AsText projections. Remove it.
- $spatialAttributes=[]; + // keep computed spatial attributes
369-369: Remove debug output leaking SQLStrip var_dump($sql); before merge.
-var_dump($sql);
2714-2741: Harden POINT decoder: type-hint, hex/binary handling, endianness-safe unpack, and error checksPrevents “array|false offset” issues and aligns with Adapter contract.
- public function decodePoint(mixed $wkb): array - { - if (str_starts_with(strtoupper($wkb), 'POINT(')) { - $start = strpos($wkb, '(') + 1; - $end = strrpos($wkb, ')'); - $inside = substr($wkb, $start, $end - $start); - $coords = explode(' ', trim($inside)); - return [(float)$coords[0], (float)$coords[1]]; - } - - // MySQL SRID-aware WKB layout: - // 1 byte = endian (1 = little endian) - // 4 bytes = type + SRID flag - // 4 bytes = SRID - // 16 bytes = X,Y coordinates (double each, little endian) - - $byteOrder = ord($wkb[0]); - $littleEndian = ($byteOrder === 1); - - // Skip 1 + 4 + 4 = 9 bytes to get coordinates - $coordsBin = substr($wkb, 9, 16); - - // Unpack doubles - $format = $littleEndian ? 'd2' : 'd2'; // little-endian doubles - $coords = unpack($format, $coordsBin); - - return [$coords[1], $coords[2]]; - } + /** + * @param string $wkb WKT "POINT(x y)" or hex/binary WKB + * @return array{0: float, 1: float} + * @throws DatabaseException + */ + public function decodePoint(string $wkb): array + { + $wkb = trim($wkb); + if (str_starts_with(strtoupper($wkb), 'POINT(')) { + $start = strpos($wkb, '('); + $end = strrpos($wkb, ')'); + if ($start === false || $end === false || $end <= $start + 1) { + throw new DatabaseException('Invalid POINT WKT'); + } + $inside = substr($wkb, $start + 1, $end - $start - 1); + $coords = preg_split('/\s+/', trim($inside)); + if (count($coords) !== 2 || !is_numeric($coords[0]) || !is_numeric($coords[1])) { + throw new DatabaseException('Invalid POINT coordinates'); + } + return [(float)$coords[0], (float)$coords[1]]; + } + + if (str_starts_with($wkb, '0x') || ctype_xdigit($wkb)) { + $bin = hex2bin(str_starts_with($wkb, '0x') ? substr($wkb, 2) : $wkb); + if ($bin === false) { + throw new DatabaseException('Invalid hex WKB'); + } + $wkb = $bin; + } + if (strlen($wkb) < 1 + 4 + 16) { + throw new DatabaseException('WKB too short for POINT'); + } + + $byteOrder = ord($wkb[0]); + $isLE = ($byteOrder === 1); + $typeArr = unpack($isLE ? 'V' : 'N', substr($wkb, 1, 4)); + if ($typeArr === false || !isset($typeArr[1])) { + throw new DatabaseException('Invalid WKB type'); + } + $typeField = $typeArr[1]; + $hasSRID = ($typeField & 0x20000000) !== 0; + $geomType = $typeField & 0xFF; + if ($geomType !== 1) { + throw new DatabaseException("Not a POINT geometry type, got {$geomType}"); + } + $offset = 5 + ($hasSRID ? 4 : 0); + if (strlen($wkb) < $offset + 16) { + throw new DatabaseException('WKB too short for POINT coords'); + } + $fmt = $isLE ? 'e' : 'E'; + $xArr = unpack($fmt, substr($wkb, $offset, 8)); + $yArr = unpack($fmt, substr($wkb, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode POINT coordinates'); + } + return [(float)$xArr[1], (float)$yArr[1]]; + }
2777-2852: Harden POLYGON decoder (hex handling, endianness-safe doubles, robust bounds)Adds PHPDoc generics and consistent offset logic with optional SRID.
- public function decodePolygon(string $wkb): array - { - // POLYGON((x1,y1),(x2,y2)) - if (str_starts_with($wkb, 'POLYGON((')) { - $start = strpos($wkb, '((') + 2; - $end = strrpos($wkb, '))'); - $inside = substr($wkb, $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); - } - - // Convert HEX string to binary if needed - if (str_starts_with($wkb, '0x') || ctype_xdigit($wkb)) { - $wkb = hex2bin(str_starts_with($wkb, '0x') ? substr($wkb, 2) : $wkb); - if ($wkb === false) { - throw new DatabaseException('Invalid hex WKB'); - } - } - - if (strlen($wkb) < 21) { - throw new DatabaseException('WKB too short to be a POLYGON'); - } - - // MySQL SRID-aware WKB layout: 4 bytes SRID prefix - $offset = 4; - - $byteOrder = ord($wkb[$offset]); - if ($byteOrder !== 1) { - throw new DatabaseException('Only little-endian WKB supported'); - } - $offset += 1; - - $type = unpack('V', substr($wkb, $offset, 4))[1]; - $hasSRID = ($type & 0x20000000) === 0x20000000; - $geomType = $type & 0xFF; - $offset += 4; - - if ($geomType !== 3) { // 3 = POLYGON - throw new DatabaseException("Not a POLYGON geometry type, got {$geomType}"); - } - - // Skip SRID in type flag if present - if ($hasSRID) { - $offset += 4; - } - - $numRings = unpack('V', substr($wkb, $offset, 4))[1]; - $offset += 4; - - $rings = []; - - for ($r = 0; $r < $numRings; $r++) { - $numPoints = unpack('V', substr($wkb, $offset, 4))[1]; - $offset += 4; - $ring = []; - - for ($p = 0; $p < $numPoints; $p++) { - $x = unpack('d', substr($wkb, $offset, 8))[1]; - $y = unpack('d', substr($wkb, $offset + 8, 8))[1]; - $ring[] = [(float)$x, (float)$y]; - $offset += 16; - } - - $rings[] = $ring; - } - - return $rings; - - } + /** + * @param string $wkb WKT "POLYGON((...))" or hex/binary WKB + * @return list<list<array{0: float, 1: float}>> + * @throws DatabaseException + */ + public function decodePolygon(string $wkb): array + { + if (str_starts_with(strtoupper($wkb), 'POLYGON((')) { + $start = strpos($wkb, '(('); + $end = strrpos($wkb, '))'); + if ($start === false || $end === false || $end <= $start + 2) { + throw new DatabaseException('Invalid POLYGON WKT'); + } + $inside = substr($wkb, $start + 2, $end - $start - 2); + $ringsStr = explode('),(', $inside); + $rings = []; + foreach ($ringsStr as $ring) { + $pts = array_map('trim', explode(',', $ring)); + $r = []; + foreach ($pts as $pt) { + $coords = preg_split('/\s+/', $pt); + if (count($coords) !== 2 || !is_numeric($coords[0]) || !is_numeric($coords[1])) { + throw new DatabaseException('Invalid point in POLYGON ring'); + } + $r[] = [(float)$coords[0], (float)$coords[1]]; + } + $rings[] = $r; + } + return $rings; + } + + if (str_starts_with($wkb, '0x') || ctype_xdigit($wkb)) { + $bin = hex2bin(str_starts_with($wkb, '0x') ? substr($wkb, 2) : $wkb); + if ($bin === false) { + throw new DatabaseException('Invalid hex WKB'); + } + $wkb = $bin; + } + if (strlen($wkb) < 1 + 4 + 4) { + throw new DatabaseException('WKB too short'); + } + + $byteOrder = ord($wkb[0]); + $isLE = ($byteOrder === 1); + $typeArr = unpack($isLE ? 'V' : 'N', substr($wkb, 1, 4)); + if ($typeArr === false || !isset($typeArr[1])) { + throw new DatabaseException('Invalid WKB type'); + } + $typeField = $typeArr[1]; + $hasSRID = ($typeField & 0x20000000) !== 0; + $geomType = $typeField & 0xFF; + if ($geomType !== 3) { + throw new DatabaseException("Not a POLYGON geometry type, got {$geomType}"); + } + + $offset = 5 + ($hasSRID ? 4 : 0); + $numRingsArr = unpack($isLE ? 'V' : 'N', substr($wkb, $offset, 4)); + if ($numRingsArr === false || !isset($numRingsArr[1])) { + throw new DatabaseException('Failed to decode POLYGON ring count'); + } + $numRings = $numRingsArr[1]; + $offset += 4; + + $fmt = $isLE ? 'e' : 'E'; + $rings = []; + for ($r = 0; $r < $numRings; $r++) { + $numPointsArr = unpack($isLE ? 'V' : 'N', substr($wkb, $offset, 4)); + if ($numPointsArr === false || !isset($numPointsArr[1])) { + throw new DatabaseException('Failed to decode POLYGON point count'); + } + $numPoints = $numPointsArr[1]; + $offset += 4; + $ring = []; + for ($p = 0; $p < $numPoints; $p++) { + if (strlen($wkb) < $offset + 16) { + throw new DatabaseException('WKB too short while reading POLYGON points'); + } + $xArr = unpack($fmt, substr($wkb, $offset, 8)); + $yArr = unpack($fmt, substr($wkb, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode POLYGON point'); + } + $ring[] = [(float)$xArr[1], (float)$yArr[1]]; + $offset += 16; + } + $rings[] = $ring; + } + return $rings; + }
2743-2775: Harden LINESTRING decoder and remove debugAdd PHPDoc generics, support hex WKB, validate unpack results, and use endian-safe doubles.
- public function decodeLinestring(string $wkb): array - { - if (str_starts_with(strtoupper($wkb), 'LINESTRING(')) { - $start = strpos($wkb, '(') + 1; - $end = strrpos($wkb, ')'); - $inside = substr($wkb, $start, $end - $start); - - $points = explode(',', $inside); - return array_map(function ($point) { - $coords = explode(' ', trim($point)); - return [(float)$coords[0], (float)$coords[1]]; - }, $points); - } - - var_dump($wkb); - - // Skip 1 byte (endianness) + 4 bytes (type) + 4 bytes (SRID) - $offset = 9; - - // Number of points (4 bytes little-endian) - $numPoints = unpack('V', substr($wkb, $offset, 4))[1]; - $offset += 4; - - $points = []; - for ($i = 0; $i < $numPoints; $i++) { - $x = unpack('d', substr($wkb, $offset, 8))[1]; - $y = unpack('d', substr($wkb, $offset + 8, 8))[1]; - $points[] = [(float)$x, (float)$y]; - $offset += 16; - } - - return $points; - } + /** + * @param string $wkb WKT "LINESTRING(...)" or hex/binary WKB + * @return list<array{0: float, 1: float}> + * @throws DatabaseException + */ + public function decodeLinestring(string $wkb): array + { + $wkb = trim($wkb); + if (str_starts_with(strtoupper($wkb), 'LINESTRING(')) { + $start = strpos($wkb, '('); + $end = strrpos($wkb, ')'); + if ($start === false || $end === false || $end <= $start + 1) { + throw new DatabaseException('Invalid LINESTRING WKT'); + } + $inside = substr($wkb, $start + 1, $end - $start - 1); + $parts = array_map('trim', explode(',', $inside)); + $out = []; + foreach ($parts as $p) { + $coords = preg_split('/\s+/', $p); + if (count($coords) !== 2 || !is_numeric($coords[0]) || !is_numeric($coords[1])) { + throw new DatabaseException('Invalid point in LINESTRING'); + } + $out[] = [(float)$coords[0], (float)$coords[1]]; + } + return $out; + } + + if (str_starts_with($wkb, '0x') || ctype_xdigit($wkb)) { + $bin = hex2bin(str_starts_with($wkb, '0x') ? substr($wkb, 2) : $wkb); + if ($bin === false) { + throw new DatabaseException('Invalid hex WKB'); + } + $wkb = $bin; + } + if (strlen($wkb) < 1 + 4 + 4) { + throw new DatabaseException('WKB too short to be a valid geometry'); + } + + $byteOrder = ord($wkb[0]); + $isLE = ($byteOrder === 1); + $typeArr = unpack($isLE ? 'V' : 'N', substr($wkb, 1, 4)); + if ($typeArr === false || !isset($typeArr[1])) { + throw new DatabaseException('Invalid WKB type'); + } + $typeField = $typeArr[1]; + $hasSRID = ($typeField & 0x20000000) !== 0; + $geomType = $typeField & 0xFF; + if ($geomType !== 2) { + throw new DatabaseException("Not a LINESTRING geometry type, got {$geomType}"); + } + + $offset = 5 + ($hasSRID ? 4 : 0); + $numPointsArr = unpack($isLE ? 'V' : 'N', substr($wkb, $offset, 4)); + if ($numPointsArr === false || !isset($numPointsArr[1])) { + throw new DatabaseException('Failed to decode LINESTRING point count'); + } + $numPoints = $numPointsArr[1]; + $offset += 4; + + $fmt = $isLE ? 'e' : 'E'; + $points = []; + for ($i = 0; $i < $numPoints; $i++) { + if (strlen($wkb) < $offset + 16) { + throw new DatabaseException('WKB too short while reading LINESTRING points'); + } + $xArr = unpack($fmt, substr($wkb, $offset, 8)); + $yArr = unpack($fmt, substr($wkb, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode LINESTRING point'); + } + $points[] = [(float)$xArr[1], (float)$yArr[1]]; + $offset += 16; + } + return $points; + }src/Database/Adapter/MariaDB.php (1)
867-867: Remove debug output leaking SQLDelete var_dump($sql); to avoid leaking queries in prod/CI.
-var_dump($sql);src/Database/Adapter.php (1)
1289-1291: Fix CodeQL by documenting shaped return types for spatial decoders and align implementations
- Add precise PHPDoc shaped return types to the three abstract methods in src/Database/Adapter.php (lines 1289–1291) — the original suggested PHPDoc is valid and should be applied to the abstract signatures.
- Mirror the same PHPDoc in each concrete implementation and ensure parameter-type intent is consistent: implementations found at src/Database/Adapter/SQL.php (decodePoint @2714, decodeLinestring @2743, decodePolygon @2777) and src/Database/Adapter/Postgres.php (decodePoint @2006, decodeLinestring @2032, decodePolygon @2090) currently use mixed for some params — either keep the abstract param as string and tighten implementations, or intentionally relax the abstract param to mixed, then add the shaped PHPDoc everywhere.
Suggested diff for Adapter.php (apply and replicate similar PHPDoc to implementations):
- abstract public function decodePoint(string $wkb): array; - abstract public function decodeLinestring(string $wkb): array; - abstract public function decodePolygon(string $wkb): array; + /** + * @param string $wkb WKT like "POINT(x y)" or hex/binary WKB + * @return array{0: float, 1: float} + */ + abstract public function decodePoint(string $wkb): array; + /** + * @param string $wkb WKT like "LINESTRING(...)" or hex/binary WKB + * @return list<array{0: float, 1: float}> + */ + abstract public function decodeLinestring(string $wkb): array; + /** + * @param string $wkb WKT like "POLYGON((...))" or hex/binary WKB + * @return list<list<array{0: float, 1: float}>> + */ + abstract public function decodePolygon(string $wkb): array;src/Database/Database.php (1)
503-507: Heads up: adapter decode paths still have var_dump noise in adaptersThis code calls adapter->decodePoint/Linestring/Polygon(). The adapter implementations still contain var_dump() per prior review; those will leak to output/logs during decode.
Run to confirm and scrub:
#!/bin/bash rg -n "var_dump\s*\(" -C2 src/Database/AdapterAlso applies to: 531-535, 559-563
src/Database/Adapter/Postgres.php (4)
996-996: Remove debug output (var_dump($sql))Leftover debug will leak SQL in production and break tests/logs. Delete it.
Apply this diff:
- var_dump($sql);
2090-2159: Fix POLYGON decoder: consistent exceptions, endian handling, float size, unsafe unpack, and debug
- Replace RuntimeException with DatabaseException.
- Remove var_dump.
- Validate hex even length and lengths before substr/unpack.
- Enforce LE or select correct double format; use 'g' (LE double).
- Add precise PHPDoc.
Apply this diff:
- public function decodePolygon(string $wkb): array + /** + * @param string $wkb WKT or WKB (hex/binary) + * @return array<int, array<int, array{0: float, 1: float}>> + * @throws DatabaseException + */ + public function decodePolygon(string $wkb): array { - // POLYGON((x1,y1),(x2,y2)) - if (str_starts_with($wkb, 'POLYGON((')) { + $wkb = trim($wkb); + // WKT + if (str_starts_with(strtoupper($wkb), 'POLYGON(')) { $start = strpos($wkb, '((') + 2; $end = strrpos($wkb, '))'); - $inside = substr($wkb, $start, $end - $start); + if ($start === false || $end === false || $end <= $start) { + throw new DatabaseException('Invalid POLYGON WKT'); + } + $inside = substr($wkb, $start, $end - $start); $rings = explode('),(', $inside); return array_map(function ($ring) { $points = explode(',', $ring); return array_map(function ($point) { $coords = explode(' ', trim($point)); + if (count($coords) !== 2 || !is_numeric($coords[0]) || !is_numeric($coords[1])) { + throw new DatabaseException('Invalid point in POLYGON ring'); + } return [(float)$coords[0], (float)$coords[1]]; }, $points); }, $rings); } - var_dump($wkb); - // Convert hex string to binary if needed - if (preg_match('/^[0-9a-fA-F]+$/', $wkb)) { - $wkb = hex2bin($wkb); - if ($wkb === false) { - throw new \RuntimeException("Invalid hex WKB"); + $bin = $wkb; + if (preg_match('/^[0-9a-fA-F]+$/', $wkb) && (strlen($wkb) % 2 === 0)) { + $bin = hex2bin($wkb); + if ($bin === false) { + throw new DatabaseException("Invalid hex WKB"); } } - if (strlen($wkb) < 9) { - throw new \RuntimeException("WKB too short"); + if (!is_string($bin) || strlen($bin) < 9) { + throw new DatabaseException("WKB too short"); } - $byteOrder = ord($wkb[0]); - $isLE = $byteOrder === 1; // assume little-endian - $uInt32 = 'V'; // little-endian 32-bit unsigned - $uDouble = 'd'; // little-endian double + $byteOrder = ord($bin[0]); + if ($byteOrder === 0) { + throw new DatabaseException("Big-endian WKB not supported"); + } elseif ($byteOrder !== 1) { + throw new DatabaseException("Invalid byte order in WKB"); + } + $uInt32 = 'V'; // little-endian 32-bit unsigned + $uDouble = 'g'; // little-endian double (64-bit) - $typeInt = unpack($uInt32, substr($wkb, 1, 4))[1]; + $typeIntArr = unpack($uInt32, substr($bin, 1, 4)); + if ($typeIntArr === false || !isset($typeIntArr[1])) { + throw new DatabaseException('Failed to decode POLYGON type'); + } + $typeInt = $typeIntArr[1]; $hasSrid = ($typeInt & 0x20000000) !== 0; $geomType = $typeInt & 0xFF; if ($geomType !== 3) { // 3 = POLYGON - throw new \RuntimeException("Not a POLYGON geometry type, got {$geomType}"); + throw new DatabaseException("Not a POLYGON geometry type, got {$geomType}"); } - $offset = 5; - if ($hasSrid) { - $offset += 4; - } + $offset = 5 + ($hasSrid ? 4 : 0); // Number of rings - $numRings = unpack($uInt32, substr($wkb, $offset, 4))[1]; + if (strlen($bin) < $offset + 4) { + throw new DatabaseException('WKB too short for POLYGON ring count'); + } + $numRingsArr = unpack($uInt32, substr($bin, $offset, 4)); + if ($numRingsArr === false || !isset($numRingsArr[1])) { + throw new DatabaseException('Failed to decode POLYGON ring count'); + } + $numRings = (int)$numRingsArr[1]; $offset += 4; $rings = []; for ($r = 0; $r < $numRings; $r++) { - $numPoints = unpack($uInt32, substr($wkb, $offset, 4))[1]; + if (strlen($bin) < $offset + 4) { + throw new DatabaseException('WKB too short for POLYGON point count'); + } + $numPointsArr = unpack($uInt32, substr($bin, $offset, 4)); + if ($numPointsArr === false || !isset($numPointsArr[1])) { + throw new DatabaseException('Failed to decode POLYGON point count'); + } + $numPoints = (int)$numPointsArr[1]; $offset += 4; $points = []; - for ($i = 0; $i < $numPoints; $i++) { - $x = unpack($uDouble, substr($wkb, $offset, 8))[1]; - $y = unpack($uDouble, substr($wkb, $offset + 8, 8))[1]; - $points[] = [(float)$x, (float)$y]; - $offset += 16; + if (strlen($bin) < $offset + (16 * $numPoints)) { + throw new DatabaseException('WKB too short for POLYGON coordinates'); + } + for ($i = 0; $i < $numPoints; $i++) { + $xArr = unpack($uDouble, substr($bin, $offset, 8)); + $yArr = unpack($uDouble, substr($bin, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode POLYGON point'); + } + $points[] = [(float)$xArr[1], (float)$yArr[1]]; + $offset += 16; } $rings[] = $points; } return $rings; // array of rings, each ring is array of [x,y] }
2032-2088: Fix LINESTRING decoder: wrong float format, unsafe unpack, stray debug, and signature mismatch
- Replace 'e' with 'g' (doubles).
- Remove var_dump.
- Validate hex, lengths, type = 2, and counts.
- Split multiple statements per line to satisfy PSR‑12.
- Signature must be string.
Apply this diff:
- public function decodeLinestring(mixed $wkb): array + /** + * @param string $wkb WKT or WKB (hex/binary) + * @return array<int, array{0: float, 1: float}> + * @throws DatabaseException + */ + public function decodeLinestring(string $wkb): array { - if (str_starts_with(strtoupper($wkb), 'LINESTRING(')) { - $start = strpos($wkb, '(') + 1; - $end = strrpos($wkb, ')'); - $inside = substr($wkb, $start, $end - $start); - - $points = explode(',', $inside); - return array_map(function ($point) { - $coords = explode(' ', trim($point)); - return [(float)$coords[0], (float)$coords[1]]; - }, $points); - } - - var_dump($wkb); - - if (ctype_xdigit($wkb)) { - $wkb = hex2bin($wkb); - } - - if (strlen($wkb) < 9) { + $wkb = trim($wkb); + if (str_starts_with(strtoupper($wkb), 'LINESTRING(')) { + $start = strpos($wkb, '('); + $end = strrpos($wkb, ')'); + if ($start === false || $end === false || $end <= $start + 1) { + throw new DatabaseException('Invalid LINESTRING WKT'); + } + $inside = substr($wkb, $start + 1, $end - $start - 1); + $parts = array_filter(array_map('trim', explode(',', $inside)), fn($p) => $p !== ''); + $out = []; + foreach ($parts as $point) { + $coords = preg_split('/\s+/', $point); + if (count($coords) !== 2 || !is_numeric($coords[0]) || !is_numeric($coords[1])) { + throw new DatabaseException('Invalid point in LINESTRING WKT'); + } + $out[] = [(float)$coords[0], (float)$coords[1]]; + } + return $out; + } + + // Hex or binary WKB + $bin = $wkb; + if (ctype_xdigit($wkb) && (strlen($wkb) % 2 === 0)) { + $bin = hex2bin($wkb); + if ($bin === false) { + throw new DatabaseException('Invalid hex WKB'); + } + } + + if (!is_string($bin) || strlen($bin) < 9) { throw new DatabaseException("WKB too short to be a valid geometry"); } - $byteOrder = ord($wkb[0]); + $byteOrder = ord($bin[0]); if ($byteOrder === 0) { throw new DatabaseException("Big-endian WKB not supported"); } elseif ($byteOrder !== 1) { throw new DatabaseException("Invalid byte order in WKB"); } // Type + SRID flag - $typeField = unpack('V', substr($wkb, 1, 4))[1]; + $typeFieldArr = unpack('V', substr($bin, 1, 4)); + if ($typeFieldArr === false || !isset($typeFieldArr[1])) { + throw new DatabaseException('Failed to decode LINESTRING type'); + } + $typeField = $typeFieldArr[1]; $geomType = $typeField & 0xFF; $hasSRID = ($typeField & 0x20000000) !== 0; if ($geomType !== 2) { // 2 = LINESTRING throw new DatabaseException("Not a LINESTRING geometry type, got {$geomType}"); } - $offset = 5; - if ($hasSRID) { - $offset += 4; - } + $offset = 5 + ($hasSRID ? 4 : 0); - $numPoints = unpack('V', substr($wkb, $offset, 4))[1]; + if (strlen($bin) < $offset + 4) { + throw new DatabaseException('WKB too short for LINESTRING point count'); + } + $numPointsArr = unpack('V', substr($bin, $offset, 4)); + if ($numPointsArr === false || !isset($numPointsArr[1])) { + throw new DatabaseException('Failed to decode LINESTRING point count'); + } + $numPoints = (int)$numPointsArr[1]; $offset += 4; + if ($numPoints < 0) { + throw new DatabaseException('Invalid LINESTRING point count'); + } + if (strlen($bin) < $offset + (16 * $numPoints)) { + throw new DatabaseException('WKB too short for LINESTRING coordinates'); + } $points = []; for ($i = 0; $i < $numPoints; $i++) { - $x = unpack('e', substr($wkb, $offset, 8))[1]; $offset += 8; - $y = unpack('e', substr($wkb, $offset, 8))[1]; $offset += 8; - $points[] = [(float)$x, (float)$y]; + $xArr = unpack('g', substr($bin, $offset, 8)); + $yArr = unpack('g', substr($bin, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode LINESTRING point'); + } + $x = (float)$xArr[1]; + $y = (float)$yArr[1]; + $points[] = [$x, $y]; + $offset += 16; } return $points; }
2006-2030: Fix POINT decoder: wrong float format, unsafe unpack, and signature mismatch
- Use 64‑bit doubles ('g'/'G'), not 32‑bit floats ('e'/'E').
- Guard hex2bin/length before ord()/substr()/unpack().
- Enforce geometry type = 1 (POINT).
- Match Adapter::decodePoint(string $wkb): array.
- Add precise return PHPDoc to satisfy CodeQL.
Apply this diff:
- public function decodePoint(mixed $wkb): array + /** + * Decode a POINT from WKT or WKB (hex or binary). + * @param string $wkb + * @return array{0: float, 1: float} + * @throws DatabaseException + */ + public function decodePoint(string $wkb): array { - //$wkb = str_replace('SRID=4326;', '', $wkb); // Remove if was added in encode - - if (str_starts_with(strtoupper($wkb), 'POINT(')) { - $start = strpos($wkb, '(') + 1; - $end = strrpos($wkb, ')'); - $inside = substr($wkb, $start, $end - $start); - - $coords = explode(' ', trim($inside)); - return [(float)$coords[0], (float)$coords[1]]; - } - - $bin = hex2bin($wkb); - - $isLE = ord($bin[0]) === 1; - $type = unpack($isLE ? 'V' : 'N', substr($bin, 1, 4))[1]; - $offset = 5 + (($type & 0x20000000) ? 4 : 0); - - $fmt = $isLE ? 'e' : 'E'; // little vs big endian double - $x = unpack($fmt, substr($bin, $offset, 8))[1]; - $y = unpack($fmt, substr($bin, $offset + 8, 8))[1]; - - return [(float)$x, (float)$y]; + $wkb = trim($wkb); + + // WKT + if (str_starts_with(strtoupper($wkb), 'POINT(')) { + $start = strpos($wkb, '('); + $end = strrpos($wkb, ')'); + if ($start === false || $end === false || $end <= $start + 1) { + throw new DatabaseException('Invalid POINT WKT'); + } + $inside = substr($wkb, $start + 1, $end - $start - 1); + $coords = preg_split('/\s+/', trim($inside)); + if (count($coords) !== 2 || !is_numeric($coords[0]) || !is_numeric($coords[1])) { + throw new DatabaseException('Invalid POINT WKT coordinates'); + } + return [(float)$coords[0], (float)$coords[1]]; + } + + // Hex or binary WKB + $bin = $wkb; + if (ctype_xdigit($wkb) && (strlen($wkb) % 2 === 0)) { + $bin = hex2bin($wkb); + if ($bin === false) { + throw new DatabaseException('Invalid hex WKB for POINT'); + } + } + if (!is_string($bin) || strlen($bin) < 1 + 4 + 16) { + throw new DatabaseException('WKB too short for POINT'); + } + + $byteOrder = ord($bin[0]); + if ($byteOrder !== 0 && $byteOrder !== 1) { + throw new DatabaseException('Invalid byte order in WKB'); + } + $isLE = ($byteOrder === 1); + + $typeArr = unpack($isLE ? 'Vtype' : 'Ntype', substr($bin, 1, 4)); + if ($typeArr === false || !isset($typeArr['type'])) { + throw new DatabaseException('Failed to decode POINT type'); + } + $type = $typeArr['type']; + $hasSRID = ($type & 0x20000000) !== 0; + $geomType = $type & 0xFF; + if ($geomType !== 1) { + throw new DatabaseException("Not a POINT geometry type, got {$geomType}"); + } + + $offset = 5 + ($hasSRID ? 4 : 0); + if (strlen($bin) < $offset + 16) { + throw new DatabaseException('WKB too short for POINT coordinates'); + } + $fmt = $isLE ? 'g' : 'G'; // little/big-endian double (64-bit) + $xArr = unpack($fmt, substr($bin, $offset, 8)); + $yArr = unpack($fmt, substr($bin, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode POINT coordinates'); + } + return [(float)$xArr[1], (float)$yArr[1]]; }
🧹 Nitpick comments (1)
src/Database/Adapter/Postgres.php (1)
2082-2083: PSR‑12: one statement per lineTwo statements per line trigger the Pint error. The diff above splits them; if you keep current structure, split them manually.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Postgres.php(2 hunks)src/Database/Adapter/SQL.php(2 hunks)src/Database/Database.php(4 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Adapter/Scopes/SpatialTests.php
🧰 Additional context used
🧬 Code graph analysis (3)
src/Database/Adapter.php (1)
src/Database/Adapter/Postgres.php (3)
decodePoint(2006-2030)decodeLinestring(2032-2088)decodePolygon(2090-2159)
src/Database/Adapter/Postgres.php (1)
src/Database/Adapter.php (3)
decodePoint(1289-1289)decodeLinestring(1290-1290)decodePolygon(1291-1291)
src/Database/Database.php (2)
src/Database/Adapter/Postgres.php (3)
decodePoint(2006-2030)decodeLinestring(2032-2088)decodePolygon(2090-2159)src/Database/Adapter.php (3)
decodePoint(1289-1289)decodeLinestring(1290-1290)decodePolygon(1291-1291)
🪛 GitHub Actions: CodeQL
src/Database/Adapter.php
[error] 1289-1289: Method Utopia\Database\Adapter::decodePoint() return type has no value type specified in iterable type array.
[error] 1290-1290: Method Utopia\Database\Adapter::decodeLinestring() return type has no value type specified in iterable type array.
[error] 1291-1291: Method Utopia\Database\Adapter::decodePolygon() return type has no value type specified in iterable type array.
src/Database/Adapter/Postgres.php
[error] 2006-2006: Utopia\Database\Adapter\Postgres::decodePoint() return type has no value type specified in iterable type array.
[error] 2021-2021: Cannot access offset 0 on string|false.
[error] 2022-2022: Cannot access offset 1 on array|false.
[error] 2022-2022: Parameter #1 $string of function substr expects string, string|false given.
[error] 2026-2026: Cannot access offset 1 on array|false.
[error] 2026-2026: Parameter #1 $string of function substr expects string, string|false given.
[error] 2027-2027: Cannot access offset 1 on array|false.
[error] 2027-2027: Parameter #1 $string of function substr expects string, string|false given.
[error] 2032-2032: Method Utopia\Database\Adapter\Postgres::decodeLinestring() return type has no value type specified in iterable type array.
[error] 2064-2064: Cannot access offset 1 on array|false.
[error] 2077-2077: Cannot access offset 1 on array|false.
[error] 2082-2082: Cannot access offset 1 on array|false.
[error] 2083-2083: Cannot access offset 1 on array|false.
[error] 2090-2090: Method Utopia\Database\Adapter\Postgres::decodePolygon() return type has no value type specified in iterable type array.
[error] 2127-2127: Cannot access offset 1 on array|false.
[error] 2141-2141: Cannot access offset 1 on array|false.
[error] 2146-2146: Cannot access offset 1 on array|false.
[error] 2150-2150: Cannot access offset 1 on array|false.
src/Database/Adapter/SQL.php
[error] 2714-2714: Method Utopia\Database\Adapter\SQL::decodePoint() return type has no value type specified in iterable type array.
[error] 2740-2740: Cannot access offset 1 on array|false.
[error] 2740-2740: Cannot access offset 2 on array|false.
[error] 2743-2743: Method Utopia\Database\Adapter\SQL::decodeLinestring() return type has no value type specified in iterable type array.
[error] 2763-2763: Cannot access offset 1 on array|false.
[error] 2768-2768: Cannot access offset 1 on array|false.
[error] 2769-2769: Cannot access offset 1 on array|false.
🪛 GitHub Actions: Linter
src/Database/Adapter/MariaDB.php
[error] 1-1: Pint PSR-12 lint error: 'statement_indentation' violation.
src/Database/Adapter/Postgres.php
[error] 1-1: Pint PSR-12 lint error: 'no_multiple_statements_per_line'.
src/Database/Database.php
[error] 1-1: Pint PSR-12 lint error: 'simplified_null_return'.
src/Database/Adapter/SQL.php
[error] 1-1: Pint PSR-12 lint error: 'statement_indentation' and 'binary_operator_space'.
🪛 PHPMD (2.15.0)
src/Database/Adapter/Postgres.php
2123-2123: Avoid unused local variables such as '$isLE'. (undefined)
(UnusedLocalVariable)
⏰ 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). (9)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/Database/Adapter/Postgres.php (5)
2006-2006: Change parameter type frommixedtostringto match interfaceThe abstract method signature in
Adapter.phpexpectsstring $wkb, but this implementation usesmixed. This violates the Liskov Substitution Principle and causes static analysis errors.-public function decodePoint(mixed $wkb): array +public function decodePoint(string $wkb): array
2032-2032: Change parameter type frommixedtostringto match interfaceSame issue as
decodePoint- the interface expectsstringbut implementation usesmixed.-public function decodeLinestring(mixed $wkb): array +public function decodeLinestring(string $wkb): array
2108-2157: Fix exception types and unsafe operations
- Using
\RuntimeExceptioninstead ofDatabaseException- Missing error handling for
unpack()operations- Unused variable
$isLE(static analysis warning)// Convert hex string to binary if needed if (preg_match('/^[0-9a-fA-F]+$/', $wkb)) { $wkb = hex2bin($wkb); if ($wkb === false) { - throw new \RuntimeException("Invalid hex WKB"); + throw new DatabaseException("Invalid hex WKB"); } } if (strlen($wkb) < 9) { - throw new \RuntimeException("WKB too short"); + throw new DatabaseException("WKB too short"); } $byteOrder = ord($wkb[0]); -$isLE = $byteOrder === 1; // assume little-endian +if ($byteOrder !== 1) { + throw new DatabaseException("Only little-endian WKB supported"); +} $uInt32 = 'V'; // little-endian 32-bit unsigned $uDouble = 'd'; // little-endian double -$typeInt = unpack($uInt32, substr($wkb, 1, 4))[1]; +$typeIntArr = unpack($uInt32, substr($wkb, 1, 4)); +if ($typeIntArr === false || !isset($typeIntArr[1])) { + throw new DatabaseException('Failed to decode POLYGON type'); +} +$typeInt = $typeIntArr[1]; if ($geomType !== 3) { // 3 = POLYGON - throw new \RuntimeException("Not a POLYGON geometry type, got {$geomType}"); + throw new DatabaseException("Not a POLYGON geometry type, got {$geomType}"); } // Number of rings -$numRings = unpack($uInt32, substr($wkb, $offset, 4))[1]; +$numRingsArr = unpack($uInt32, substr($wkb, $offset, 4)); +if ($numRingsArr === false || !isset($numRingsArr[1])) { + throw new DatabaseException('Failed to decode ring count'); +} +$numRings = $numRingsArr[1]; - $numPoints = unpack($uInt32, substr($wkb, $offset, 4))[1]; + $numPointsArr = unpack($uInt32, substr($wkb, $offset, 4)); + if ($numPointsArr === false || !isset($numPointsArr[1])) { + throw new DatabaseException('Failed to decode point count'); + } + $numPoints = $numPointsArr[1]; - $x = unpack($uDouble, substr($wkb, $offset, 8))[1]; - $y = unpack($uDouble, substr($wkb, $offset + 8, 8))[1]; + $xArr = unpack($uDouble, substr($wkb, $offset, 8)); + $yArr = unpack($uDouble, substr($wkb, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode point coordinates'); + } + $x = $xArr[1]; + $y = $yArr[1];
2019-2030: Fix multiple issues in WKB decoding logic
- Missing error handling for
hex2bin()failure- Unguarded
unpack()operations that can returnfalse- Incorrect float unpacking format - using 32-bit format ('e') while consuming 8 bytes
-$bin = hex2bin($wkb); +$bin = hex2bin($wkb); +if ($bin === false || strlen($bin) < 21) { + throw new DatabaseException('Invalid WKB data'); +} $isLE = ord($bin[0]) === 1; -$type = unpack($isLE ? 'V' : 'N', substr($bin, 1, 4))[1]; +$typeArr = unpack($isLE ? 'V' : 'N', substr($bin, 1, 4)); +if ($typeArr === false || !isset($typeArr[1])) { + throw new DatabaseException('Failed to decode geometry type'); +} +$type = $typeArr[1]; $offset = 5 + (($type & 0x20000000) ? 4 : 0); -$fmt = $isLE ? 'e' : 'E'; // little vs big endian double -$x = unpack($fmt, substr($bin, $offset, 8))[1]; -$y = unpack($fmt, substr($bin, $offset + 8, 8))[1]; +$fmt = $isLE ? 'd' : 'D'; // 64-bit double format +$xArr = unpack($fmt, substr($bin, $offset, 8)); +$yArr = unpack($fmt, substr($bin, $offset + 8, 8)); +if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode coordinates'); +} +$x = $xArr[1]; +$y = $yArr[1];
2046-2088: Fix unsafe operations and incorrect format specifierMultiple issues similar to
decodePoint:
- No error handling for
hex2bin()failure- Unguarded
unpack()operations- Incorrect float format specifier
if (ctype_xdigit($wkb)) { $wkb = hex2bin($wkb); + if ($wkb === false) { + throw new DatabaseException('Invalid hex WKB'); + } } // Type + SRID flag -$typeField = unpack('V', substr($wkb, 1, 4))[1]; +$typeFieldArr = unpack('V', substr($wkb, 1, 4)); +if ($typeFieldArr === false || !isset($typeFieldArr[1])) { + throw new DatabaseException('Failed to decode LINESTRING type'); +} +$typeField = $typeFieldArr[1]; -$numPoints = unpack('V', substr($wkb, $offset, 4))[1]; +$numPointsArr = unpack('V', substr($wkb, $offset, 4)); +if ($numPointsArr === false || !isset($numPointsArr[1])) { + throw new DatabaseException('Failed to decode point count'); +} +$numPoints = $numPointsArr[1]; $points = []; for ($i = 0; $i < $numPoints; $i++) { - $x = unpack('e', substr($wkb, $offset, 8))[1]; - $offset += 8; - $y = unpack('e', substr($wkb, $offset, 8))[1]; - $offset += 8; + $xArr = unpack('d', substr($wkb, $offset, 8)); + $yArr = unpack('d', substr($wkb, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode point coordinates'); + } + $x = $xArr[1]; + $y = $yArr[1]; $points[] = [(float)$x, (float)$y]; + $offset += 16; }
🧹 Nitpick comments (2)
src/Database/Adapter/Postgres.php (2)
2006-2157: Add missing PHPDoc blocks with proper type hintsAll three decode methods need PHPDoc blocks to satisfy static analysis requirements and improve developer experience.
+/** + * Decode a POINT geometry from WKT or WKB format + * @param string $wkb WKT string or WKB hex string + * @return array{0: float, 1: float} Point coordinates [x, y] + * @throws DatabaseException + */ public function decodePoint(string $wkb): array +/** + * Decode a LINESTRING geometry from WKT or WKB format + * @param string $wkb WKT string or WKB hex string + * @return array<int, array{0: float, 1: float}> Array of point coordinates + * @throws DatabaseException + */ public function decodeLinestring(string $wkb): array +/** + * Decode a POLYGON geometry from WKT or WKB format + * @param string $wkb WKT string or WKB hex string + * @return array<int, array<int, array{0: float, 1: float}>> Array of rings, each containing points + * @throws DatabaseException + */ public function decodePolygon(string $wkb): array
2010-2017: Add input validation for WKT parsingThe WKT parsing sections lack validation for coordinate format, which could cause errors with malformed input.
if (str_starts_with(strtoupper($wkb), 'POINT(')) { $start = strpos($wkb, '(') + 1; $end = strrpos($wkb, ')'); $inside = substr($wkb, $start, $end - $start); $coords = explode(' ', trim($inside)); + if (count($coords) !== 2 || !is_numeric($coords[0]) || !is_numeric($coords[1])) { + throw new DatabaseException('Invalid POINT format in WKT'); + } return [(float)$coords[0], (float)$coords[1]]; }Apply similar validation to the LINESTRING and POLYGON WKT parsing sections.
Also applies to: 2034-2044, 2093-2106
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Database/Adapter/Postgres.php(1 hunks)src/Database/Adapter/SQL.php(2 hunks)src/Database/Database.php(3 hunks)tests/e2e/Adapter/Scopes/SpatialTests.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Database/Adapter/SQL.php
- tests/e2e/Adapter/Scopes/SpatialTests.php
- src/Database/Database.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/Postgres.php (2)
src/Database/Adapter/SQL.php (3)
decodePoint(2714-2741)decodeLinestring(2743-2773)decodePolygon(2775-2850)src/Database/Adapter.php (3)
decodePoint(1289-1289)decodeLinestring(1290-1290)decodePolygon(1291-1291)
🪛 PHPMD (2.15.0)
src/Database/Adapter/Postgres.php
2121-2121: Avoid unused local variables such as '$isLE'. (undefined)
(UnusedLocalVariable)
🪛 GitHub Actions: CodeQL
src/Database/Adapter/Postgres.php
[error] 2006-2006: Method Utopia\Database\Adapter\Postgres::decodePoint() return type has no value type specified in iterable type array.
[error] 2021-2021: Cannot access offset 0 on string|false.
[error] 2022-2022: Cannot access offset 1 on array|false.
[error] 2022-2022: Parameter #1 $string of function substr expects string, string|false given.
[error] 2026-2026: Cannot access offset 1 on array|false.
[error] 2026-2026: Parameter #1 $string of function substr expects string, string|false given.
[error] 2027-2027: Cannot access offset 1 on array|false.
[error] 2027-2027: Parameter #1 $string of function substr expects string, string|false given.
[error] 2032-2032: Method Utopia\Database\Adapter\Postgres::decodeLinestring() return type has no value type specified in iterable type array.
[error] 2062-2062: Cannot access offset 1 on array|false.
[error] 2075-2075: Cannot access offset 1 on array|false.
[error] 2080-2080: Cannot access offset 1 on array|false.
[error] 2082-2082: Cannot access offset 1 on array|false.
[error] 2090-2090: Method Utopia\Database\Adapter\Postgres::decodePolygon() return type has no value type specified in iterable type array.
[error] 2125-2125: Cannot access offset 1 on array|false.
[error] 2139-2139: Cannot access offset 1 on array|false.
[error] 2144-2144: Cannot access offset 1 on array|false.
[error] 2149-2149: Cannot access offset 1 on array|false.
⏰ 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). (2)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Adapter/SQL.php (1)
519-527: Avoid misclassifying spatial attrs during update (json-encoding bug).isset($spatialAttributes[$attributeName]) is wrong for a list; this will JSON‑encode spatial arrays.
- foreach ($attributes as $attributeName => $value) { - if (!isset($spatialAttributes[$attributeName]) && is_array($value)) { + foreach ($attributes as $attributeName => $value) { + if (!in_array($attributeName, $spatialAttributes, true) && is_array($value)) { $value = json_encode($value); }
♻️ Duplicate comments (4)
src/Database/Adapter/SQL.php (1)
362-362: Don’t wipe spatial projection.Overriding $spatialAttributes disables ST_AsText in getDocument(). Remove the reset.
- $spatialAttributes = [];src/Database/Adapter/Postgres.php (3)
2030-2086: Align signature with Adapter and harden hex handling in decodeLinestring.Change mixed→string and guard hex2bin(); fixes LSP and static analysis.
- public function decodeLinestring(mixed $wkb): array + public function decodeLinestring(string $wkb): array { if (str_starts_with(strtoupper($wkb), 'LINESTRING(')) { @@ - if (ctype_xdigit($wkb)) { - $wkb = hex2bin($wkb); - } + if (ctype_xdigit($wkb)) { + $wkb = hex2bin($wkb); + if ($wkb === false) { + throw new DatabaseException('Invalid hex WKB'); + } + }
2006-2028: Guard hex2bin and array offsets in decodePoint.Fix CodeQL errors by validating hex2bin() and lengths before indexing/unpack.
public function decodePoint(string $wkb): array { if (str_starts_with(strtoupper($wkb), 'POINT(')) { @@ } - $bin = hex2bin($wkb); + $bin = hex2bin($wkb); + if ($bin === false || strlen($bin) < 21) { + throw new DatabaseException('Invalid hex WKB for POINT'); + } - $isLE = ord($bin[0]) === 1; - $type = unpack($isLE ? 'V' : 'N', substr($bin, 1, 4))[1]; + $isLE = ord($bin[0]) === 1; + $typeArr = unpack($isLE ? 'V' : 'N', substr($bin, 1, 4)); + if ($typeArr === false || !isset($typeArr[1])) { + throw new DatabaseException('Failed to decode POINT type'); + } + $type = $typeArr[1]; $offset = 5 + (($type & 0x20000000) ? 4 : 0); - $fmt = $isLE ? 'e' : 'E'; // little vs big endian double - $x = unpack($fmt, substr($bin, $offset, 8))[1]; - $y = unpack($fmt, substr($bin, $offset + 8, 8))[1]; + $fmt = $isLE ? 'e' : 'E'; // little vs big endian double + $xArr = unpack($fmt, substr($bin, $offset, 8)); + $yArr = unpack($fmt, substr($bin, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Failed to decode POINT coordinates'); + } - return [(float)$x, (float)$y]; + return [(float)$xArr[1], (float)$yArr[1]]; }
2106-2154: Use DatabaseException and enforce endianness in decodePolygon; drop unused var.Replace RuntimeException, validate byte order, and remove $isLE PHPMD warning.
- if (preg_match('/^[0-9a-fA-F]+$/', $wkb)) { + if (preg_match('/^[0-9a-fA-F]+$/', $wkb)) { $wkb = hex2bin($wkb); if ($wkb === false) { - throw new \RuntimeException("Invalid hex WKB"); + throw new DatabaseException("Invalid hex WKB"); } } if (strlen($wkb) < 9) { - throw new \RuntimeException("WKB too short"); + throw new DatabaseException("WKB too short"); } $byteOrder = ord($wkb[0]); - $isLE = $byteOrder === 1; // assume little-endian + if ($byteOrder !== 1) { + throw new DatabaseException("Only little-endian WKB supported"); + } $uInt32 = 'V'; // little-endian 32-bit unsigned - $uDouble = 'd'; // little-endian double + $uDouble = 'e'; // little-endian double - $typeInt = unpack($uInt32, substr($wkb, 1, 4))[1]; + $typeIntArr = unpack($uInt32, substr($wkb, 1, 4)); + if ($typeIntArr === false || !isset($typeIntArr[1])) { + throw new DatabaseException('Failed to decode POLYGON type'); + } + $typeInt = $typeIntArr[1]; $hasSrid = ($typeInt & 0x20000000) !== 0; $geomType = $typeInt & 0xFF; if ($geomType !== 3) { // 3 = POLYGON - throw new \RuntimeException("Not a POLYGON geometry type, got {$geomType}"); + throw new DatabaseException("Not a POLYGON geometry type, got {$geomType}"); } @@ - $numRings = unpack($uInt32, substr($wkb, $offset, 4))[1]; + $numRingsArr = unpack($uInt32, substr($wkb, $offset, 4)); + if ($numRingsArr === false || !isset($numRingsArr[1])) { + throw new DatabaseException('Failed to decode POLYGON ring count'); + } + $numRings = $numRingsArr[1]; $offset += 4; @@ - $numPoints = unpack($uInt32, substr($wkb, $offset, 4))[1]; + $numPointsArr = unpack($uInt32, substr($wkb, $offset, 4)); + if ($numPointsArr === false || !isset($numPointsArr[1])) { + throw new DatabaseException('Failed to decode POLYGON point count'); + } + $numPoints = $numPointsArr[1];
🧹 Nitpick comments (1)
tests/e2e/Adapter/Base.php (1)
21-26: Fix PSR-12 indentation for commented trait lines (or remove them).They’re starting at column 0 inside the class, tripping Pint. Indent them to 4 spaces.
-// use CollectionTests; -// use DocumentTests; -// use AttributeTests; -// use IndexTests; -// use PermissionTests; -// use RelationshipTests; + // use CollectionTests; + // use DocumentTests; + // use AttributeTests; + // use IndexTests; + // use PermissionTests; + // use RelationshipTests;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Database/Adapter.php(1 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/Postgres.php(1 hunks)src/Database/Adapter/SQL.php(2 hunks)tests/e2e/Adapter/Base.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Adapter.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Database/Adapter/SQL.php (2)
src/Database/Adapter.php (3)
decodePoint(1295-1295)decodeLinestring(1303-1303)decodePolygon(1311-1311)src/Database/Adapter/Postgres.php (3)
decodePoint(2006-2028)decodeLinestring(2030-2086)decodePolygon(2088-2155)
src/Database/Adapter/Postgres.php (2)
src/Database/Adapter.php (3)
decodePoint(1295-1295)decodeLinestring(1303-1303)decodePolygon(1311-1311)src/Database/Adapter/SQL.php (3)
decodePoint(2714-2745)decodeLinestring(2747-2787)decodePolygon(2789-2885)
🪛 GitHub Actions: Linter
tests/e2e/Adapter/Base.php
[error] 1-1: Pint: PSR-12 statement_indentation violation detected in tests/e2e/Adapter/Base.php.
🪛 PHPMD (2.15.0)
src/Database/Adapter/Postgres.php
2119-2119: Avoid unused local variables such as '$isLE'. (undefined)
(UnusedLocalVariable)
🪛 GitHub Actions: CodeQL
src/Database/Adapter/Postgres.php
[error] 2019-2019: Cannot access offset 0 on string|false.
[error] 2020-2020: Cannot access offset 1 on array|false.
[error] 2020-2020: Parameter #1 $string of function substr expects string, string|false given.
[error] 2024-2024: Cannot access offset 1 on array|false.
[error] 2024-2024: Parameter #1 $string of function substr expects string, string|false given.
[error] 2025-2025: Cannot access offset 1 on array|false.
[error] 2025-2025: Parameter #1 $string of function substr expects string, string|false given.
[error] 2048-2048: Parameter #1 $string of function strlen expects string, string|false given.
[error] 2052-2052: Cannot access offset 0 on string|false.
[error] 2060-2060: Cannot access offset 1 on array|false.
[error] 2060-2060: Parameter #1 $string of function substr expects string, string|false given.
[error] 2073-2073: Cannot access offset 1 on array|false.
[error] 2073-2073: Parameter #1 $string of function substr expects string, string|false given.
[error] 2078-2078: Cannot access offset 1 on array|false.
[error] 2078-2078: Parameter #1 $string of function substr expects string, string|false given.
[error] 2080-2080: Cannot access offset 1 on array|false.
[error] 2080-2080: Parameter #1 $string of function substr expects string, string|false given.
[error] 2123-2123: Cannot access offset 1 on array|false.
[error] 2137-2137: Cannot access offset 1 on array|false.
[error] 2142-2142: Cannot access offset 1 on array|false.
[error] 2146-2146: Cannot access offset 1 on array|false.
[error] 2147-2147: Cannot access offset 1 on array|false.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Unit Test
🔇 Additional comments (1)
src/Database/Adapter/Pool.php (1)
553-566: Delegation for spatial decoders looks good.Signatures match Adapter contract and delegate correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Database/Adapter/SQL.php (3)
2458-2461: Fix broken call: getAttributeProjection now has 2 paramsThis is the source of the pipeline failure. Update the call to match the new signature.
Apply this diff:
- SELECT {$this->getAttributeProjection($selections, $alias, $spatialAttributes)} + SELECT {$this->getAttributeProjection($selections, $alias)}
518-527: Spatial attribute check uses isset on a list — switch to in_array
isset($spatialAttributes[$attributeName])is wrong because$spatialAttributesis a list of names. This will JSON‑encode spatial values by mistake.- if (!isset($spatialAttributes[$attributeName]) && is_array($value)) { + if (!in_array($attributeName, $spatialAttributes, true) && is_array($value)) { $value = json_encode($value); }
2458-2465: Fix mismatched getAttributeProjection call (3rd arg $spatialAttributes)3‑arg invocation found at src/Database/Adapter/SQL.php:2459; the abstract signature (src/Database/Adapter.php:1158) and current implementation (src/Database/Adapter/SQL.php:1882) accept two parameters. Remove the third argument from the call site or update the abstract and implementations to accept and handle $spatialAttributes.
♻️ Duplicate comments (2)
src/Database/Adapter/SQL.php (2)
2716-2756: LINESTRING WKB: honor byte order, handle hex, parse type/SRIDAssumes little‑endian, fixed SRID layout, and machine‑endian doubles; can misdecode or throw on valid inputs.
public function decodeLinestring(string $wkb): array { if (str_starts_with(strtoupper($wkb), 'LINESTRING(')) { $start = strpos($wkb, '(') + 1; $end = strrpos($wkb, ')'); $inside = substr($wkb, $start, $end - $start); $points = explode(',', $inside); return array_map(function ($point) { $coords = explode(' ', trim($point)); return [(float)$coords[0], (float)$coords[1]]; }, $points); } - // Skip 1 byte (endianness) + 4 bytes (type) + 4 bytes (SRID) - $offset = 9; - - // Number of points (4 bytes little-endian) - $numPointsArr = unpack('V', substr($wkb, $offset, 4)); + if (str_starts_with($wkb, '0x') || ctype_xdigit($wkb)) { + $wkb = hex2bin(str_starts_with($wkb, '0x') ? substr($wkb, 2) : $wkb); + if ($wkb === false) { + throw new DatabaseException('Invalid hex WKB for LINESTRING'); + } + } + if (!is_string($wkb) || strlen($wkb) < 1 + 4 + 4) { + throw new DatabaseException('WKB too short to be a LINESTRING'); + } + $byteOrder = ord($wkb[0]); + if ($byteOrder !== 0 && $byteOrder !== 1) { + throw new DatabaseException('Invalid byte order in WKB'); + } + $le = ($byteOrder === 1); + $u32 = $le ? 'V' : 'N'; + $f64 = $le ? 'e' : 'E'; + + // Type + SRID flag + $typeArr = unpack($u32, substr($wkb, 1, 4)); + if ($typeArr === false || !isset($typeArr[1])) { + throw new DatabaseException('Invalid WKB: cannot unpack geometry type'); + } + $type = (int)$typeArr[1]; + $geomType = $type & 0xFF; + if ($geomType !== 2) { + throw new DatabaseException("Not a LINESTRING geometry type, got {$geomType}"); + } + $hasSRID = ($type & 0x20000000) !== 0; + + $offset = 1 + 4 + ($hasSRID ? 4 : 0); + + $numPointsArr = unpack($u32, substr($wkb, $offset, 4)); if ($numPointsArr === false || !isset($numPointsArr[1])) { throw new DatabaseException('Invalid WKB: cannot unpack number of points'); } $numPoints = $numPointsArr[1]; $offset += 4; $points = []; for ($i = 0; $i < $numPoints; $i++) { - $xArr = unpack('d', substr($wkb, $offset, 8)); - $yArr = unpack('d', substr($wkb, $offset + 8, 8)); + if (strlen($wkb) < $offset + 16) { + throw new DatabaseException('WKB too short for LINESTRING point'); + } + $xArr = unpack($f64, substr($wkb, $offset, 8)); + $yArr = unpack($f64, substr($wkb, $offset + 8, 8)); if ($xArr === false || !isset($xArr[1]) || $yArr === false || !isset($yArr[1])) { throw new DatabaseException('Invalid WKB: cannot unpack point coordinates'); } $points[] = [(float)$xArr[1], (float)$yArr[1]]; $offset += 16; } return $points; }
2683-2714: POINT WKB: add hex handling, endianness, SRID offset, and input validationCurrent code assumes little‑endian machine doubles (
'd'), doesn’t parse the type/SRID flag, and can’t accept hex input. This will decode incorrectly for big‑endian WKB or when SRID is absent/present.public function decodePoint(string $wkb): array { if (str_starts_with(strtoupper($wkb), 'POINT(')) { $start = strpos($wkb, '(') + 1; $end = strrpos($wkb, ')'); $inside = substr($wkb, $start, $end - $start); $coords = explode(' ', trim($inside)); return [(float)$coords[0], (float)$coords[1]]; } - // MySQL SRID-aware WKB layout: - // 1 byte = endian (1 = little endian) - // 4 bytes = type + SRID flag - // 4 bytes = SRID - // 16 bytes = X,Y coordinates (double each, little endian) - - $byteOrder = ord($wkb[0]); - $littleEndian = ($byteOrder === 1); - - // Skip 1 + 4 + 4 = 9 bytes to get coordinates - $coordsBin = substr($wkb, 9, 16); - - // Unpack doubles - $format = $littleEndian ? 'd2' : 'd2'; // little-endian doubles - $coords = unpack($format, $coordsBin); - - if ($coords === false || !isset($coords[1], $coords[2])) { - throw new DatabaseException('Invalid WKB for POINT: cannot unpack coordinates'); - } - - return [(float)$coords[1], (float)$coords[2]]; + // Accept hex WKB as well as binary + if (str_starts_with($wkb, '0x') || ctype_xdigit($wkb)) { + $wkb = hex2bin(str_starts_with($wkb, '0x') ? substr($wkb, 2) : $wkb); + if ($wkb === false) { + throw new DatabaseException('Invalid hex WKB for POINT'); + } + } + if (!is_string($wkb) || strlen($wkb) < 1 + 4 + 16) { + throw new DatabaseException('WKB too short for POINT'); + } + + $byteOrder = ord($wkb[0]); + if ($byteOrder !== 0 && $byteOrder !== 1) { + throw new DatabaseException('Invalid byte order in WKB'); + } + $le = ($byteOrder === 1); + $u32 = $le ? 'V' : 'N'; + $f64 = $le ? 'e' : 'E'; + + // Read type field and optional SRID + $typeArr = unpack($u32, substr($wkb, 1, 4)); + if ($typeArr === false || !isset($typeArr[1])) { + throw new DatabaseException('Invalid WKB: cannot unpack geometry type'); + } + $type = (int)$typeArr[1]; + $hasSRID = ($type & 0x20000000) !== 0; + + $offset = 1 + 4 + ($hasSRID ? 4 : 0); + if (strlen($wkb) < $offset + 16) { + throw new DatabaseException('WKB too short for POINT coordinates'); + } + + $xArr = unpack($f64, substr($wkb, $offset, 8)); + $yArr = unpack($f64, substr($wkb, $offset + 8, 8)); + if ($xArr === false || $yArr === false || !isset($xArr[1], $yArr[1])) { + throw new DatabaseException('Invalid WKB for POINT: cannot unpack coordinates'); + } + return [(float)$xArr[1], (float)$yArr[1]]; }
🧹 Nitpick comments (2)
src/Database/Adapter/SQL.php (2)
39-42: Make float formatting locale-independent
sprintf('%.…F', …)can be locale-sensitive; databases expect a dot decimal separator. Use a locale‑independent formatter.- return sprintf('%.'. $this->floatPrecision . 'F', $value); + return number_format($value, $this->floatPrecision, '.', '');
541-606: Nit: unused variable $removeBindKeys
$removeBindKeysis appended to but never read. Clean up to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/Pool.php(2 hunks)src/Database/Adapter/SQL.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Adapter.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Database/Adapter/Pool.php (4)
src/Database/Adapter.php (4)
getAttributeProjection(1158-1158)decodePoint(1295-1295)decodeLinestring(1303-1303)decodePolygon(1311-1311)src/Database/Adapter/SQL.php (4)
getAttributeProjection(1882-1911)decodePoint(2683-2714)decodeLinestring(2716-2756)decodePolygon(2758-2854)src/Database/Adapter/Postgres.php (3)
decodePoint(2006-2053)decodeLinestring(2055-2144)decodePolygon(2146-2248)src/Database/Mirror.php (1)
delegate(88-103)
src/Database/Adapter/SQL.php (4)
src/Database/Adapter.php (5)
getAttributeProjection(1158-1158)quote(468-468)decodePoint(1295-1295)decodeLinestring(1303-1303)decodePolygon(1311-1311)src/Database/Adapter/Pool.php (5)
getAttributeProjection(473-476)quote(118-121)decodePoint(547-550)decodeLinestring(552-555)decodePolygon(557-560)src/Database/Adapter/Postgres.php (4)
quote(1941-1944)decodePoint(2006-2053)decodeLinestring(2055-2144)decodePolygon(2146-2248)src/Database/Adapter/MariaDB.php (1)
quote(1825-1828)
🪛 GitHub Actions: CodeQL
src/Database/Adapter/SQL.php
[error] 2459-2459: PHPStan: Method Utopia\Database\Adapter\SQL::getAttributeProjection() invoked with 3 parameters, 2 required.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Unit Test
- GitHub Check: Adapter Tests (SharedTables/Postgres)
🔇 Additional comments (4)
src/Database/Adapter/SQL.php (2)
363-371: Projection behavior change: confirm downstream spatial decodingST_AsText-based spatial projections were removed. Ensure the code that consumes rows from getDocument() decodes spatial columns via the new adapter decoders before returning to callers.
If needed, I can help wire decoding into the fetch->Document transformation path.
2466-2482: Bind doubles as strings: confirm precision vs server settingsSwitching to stringified doubles is good; verify MySQL/MariaDB SQL modes won’t reject very long decimals and that precision aligns with index usage if any sorted by floats.
src/Database/Adapter/Pool.php (2)
473-476: Signature sync with Adapter/SQL: LGTMDelegation matches the updated 2‑arg
getAttributeProjectionsignature.
547-560: Decoder delegation added: LGTMForwarders for
decodePoint/Linestring/Polygonare correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Database/Adapter/SQL.php (1)
2850-2859: Inconsistent exception types in Polygon decoder.Lines 2851 and 2858 throw
\RuntimeExceptionwhile the rest of the spatial decoders consistently throwDatabaseException. This creates inconsistent error handling.Apply this fix for consistency:
- if ($xArr === false) { - throw new \RuntimeException('Failed to unpack X coordinate from WKB.'); - } + if ($xArr === false) { + throw new DatabaseException('Failed to unpack X coordinate from WKB.'); + } - if ($yArr === false) { - throw new \RuntimeException('Failed to unpack Y coordinate from WKB.'); - } + if ($yArr === false) { + throw new DatabaseException('Failed to unpack Y coordinate from WKB.'); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/Postgres.php(1 hunks)src/Database/Adapter/SQL.php(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Database/Adapter/Postgres.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/SQL.php (4)
src/Database/Adapter.php (5)
getAttributeProjection(1158-1158)quote(468-468)decodePoint(1295-1295)decodeLinestring(1303-1303)decodePolygon(1311-1311)src/Database/Adapter/Pool.php (5)
getAttributeProjection(473-476)quote(118-121)decodePoint(547-550)decodeLinestring(552-555)decodePolygon(557-560)src/Database/Adapter/Postgres.php (4)
quote(1941-1944)decodePoint(2006-2048)decodeLinestring(2050-2132)decodePolygon(2134-2226)src/Database/Adapter/MariaDB.php (1)
quote(1825-1828)
⏰ 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/MySQL)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MariaDB)
🔇 Additional comments (1)
src/Database/Adapter/SQL.php (1)
2682-2871: Align WKB decoding and error handling between SQL and Postgres adapters
- Observed differences: SQL (src/Database/Adapter/SQL.php) accepts WKT text (e.g. "POINT("), assumes MySQL-style SRID prefix, only little-endian, and throws DatabaseException; Postgres (src/Database/Adapter/Postgres.php) performs hex→binary conversion, has more granular byte-order/type/unpack checks and throws \RuntimeException.
- Action: unify supported input formats (binary WKB, hex WKB, WKT), decide and document SRID semantics (4‑byte prefix vs type-flag), standardize endianness handling (accept/explicitly reject big-endian) and exception types/messages across both adapters. Add cross-adapter unit tests covering: little-endian/ big-endian, SRID/no-SRID, hex vs binary, and WKT forms.
Summary by CodeRabbit