Repository navigation
fix(validator): validate spatial filter coordinates and parse WKT - #1029
Merged
Merged
Conversation
Filter only asked a spatial filter's value to be an array. A polygon nested too deep or a point out of range reached the engine and failed there, which MySQL raised as error 3037, 3616 or 3617 and nothing mapped, and a point of strings was cast to (0, 0) by the builder and silently matched against the origin. The Spatial validator of stored values read an object's coordinates by position and raised an undefined key warning. 7.4.2 checks both the same way. A spatial filter's geometry, and a distance query's unless it is WKT, is now read in the shape the builders read it, a point, a linestring or a polygon, and held to the rules of a stored value of that type: numeric coordinates in longitude and latitude range, and closed rings of at least four points. Spatial refuses a coordinate, point or ring given as an object, and the GIS errors map to Exception\Query: ER_GIS_INVALID_DATA on both engines, and MySQL's longitude and latitude range errors on MySQL only, since MariaDB reads such a geometry as NULL. Two E2E queries leaned on the loose check: a linestring wrapped one level too deep, which the builder read as a polygon of one two-point ring, and a point at latitude 100. They now pass well-formed geometry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… finite The Spatial validator took any string that began with POINT(, LINESTRING( or POLYGON( as WKT, so a written value such as `POINT(1 2` or `POINT(200 0)` reached the engine: MySQL raised error 3037, a server error, and MariaDB stored it and warned on the read back. A distance query from WKT was checked the same way, so its coordinates were never held to their range. A coordinate of INF or NAN passed the range check, which no comparison with NAN can fail. WKT is now parsed, without a regex, into the coordinates of its type and held to the rules of a value of that type: a point of two numbers, closed rings of at least four points, coordinates in longitude and latitude range. A written value must be WKT of the attribute's own type, and is refused as an invalid structure otherwise, so Appwrite answers 400 rather than 500. A distance query from WKT is validated as its geometry is. A coordinate that is not finite is refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 48.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 45 functions across 10 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main changes: spatial filter coordinate validation and WKT parsing. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
These are two staging 500s on spatial columns, A and B.
A. A spatial read filter with out-of-range coordinates reached the engine.
intersects("pt", [[200,100]])failed on MySQL withPDOException 3616/3617inst_geomfromtext, which is a 500.Filteronly checked that a spatial filter's value was an array. Ranges were checked on writes only.intersects/crosses/overlaps/touches/covers/spatialEqualsand their negations,equal/notEqual,contains, and thedistance*methods.INFandNANcoordinates are refused. A comparison withNANnever fails, soNANused to pass the range check.Exception\Queryon reads:ER_GIS_INVALID_DATA(3037) on MySQL and MariaDB, and 3616/3617 on MySQL.B. Writing malformed WKT gave a 500. Writing
{"pt":"POINT(1 2"}failed with MySQL 3037 or a PostgreSQL parse error, and MariaDB stored the value and then warnedUndefined array key 1when reading it back.Spatial::isWktString()only checked the prefix with a regex.Exception\Structure. Appwrite's create and update endpoints map that to400 row_invalid_structureand do not catchException\Query.The first commit is ported from the unpushed
qfix-overflowwork (502a028fa). The second adds the WKT parser, the write-side and non-finite refusals, and the tests.Regression tests
SpatialTests::testSpatialFilterOutOfRangeOrMalformedIsAnInvalidQuery: (200,100); lon ±180.0001; lat ±90.0001; (1e308,1e308); INF; NAN; polygon at lat 91; line at lon 181;equalout of range; distance from a point or from WKT out of range; distance from malformed WKT; each onfind()andcount(); edges (±180, ±90, a polygon touching lat 90, a WKT distance) still workPDOException 3616 Longitude 200.000000 is out of rangeSpatialTests::testSpatialWriteOfMalformedOrOutOfRangeValueIsAnInvalidStructure:POINT(1 2,POINT(200 0),POINT(0 NAN),POINT(1 2 3), WKT of another type, trailing text, out-of-range or non-finite arrays, a line at lon 181, a polygon at lat 91, an unclosed polygon and an unclosed ring, each on create and update; WKT at ±180/±90 and a polygon with a hole written and read backPDOException 3037 Invalid GIS dataPOINT(1 2accepted and stored, then warningUndefined array key 1atMariaDB.php:845PDOException XX000 parse error - invalid geometryUnit\Validator\SpatialTestmalformed WKT (15 cases), parsing, type rules, non-finiteUnit\Validator\Query\FilterGeometryTest,Unit\EngineErrorMappingTestGIS casesGeometryvalidator, unmapped errors)The fix commit alone without the WKT parser was also checked: MySQL still accepted a WKT distance from (200,100), and writes of
POINT(1 2came back asException\Query(a 500 in Appwrite) or were stored on MariaDB. That is why the second commit exists.Run locally against Docker: the full adapter suites for MySQL, MariaDB, Postgres, SQLite, MongoDB, Memory and Redis pass, as do the full unit suite, Pint and PHPStan.
API repros (Appwrite)
GET /v1/tablesdb/{db}/tables/{t}/rows?queries[]={"method":"intersects","attribute":"pt","values":[[200,100]]}→400 general_query_invalid(was 500). The same holds for[[180.0001,0]],[[1e308,1e308]], a polygon at latitude 91 on a polygon column, a line at longitude 181 withcrosses, and{"method":"distanceLessThan","attribute":"pt","values":[[[200,100],10,false]]}.POST /v1/tablesdb/{db}/tables/{t}/rowswith body{"rowId":"unique()","data":{"pt":"POINT(1 2"}}→400 row_invalid_structure(was 500).PATCH .../rows/{rowId}with{"data":{"pt":"POINT(200 0)"}}→400 row_invalid_structure. The Collections API returnsdocument_invalid_structure.🤖 Generated with Claude Code
Summary by CodeRabbit