developmentseed / developmentseed/titiler-covjson
Parse a multipoint per point instead of flattening its body
- Dominant language
- Python
- Stars
- 1
- Forks
- 1
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 12
Description
`parse_multipoint_wkt` validates its point list with two ad-hoc guards and then throws the structure away by flattening, which costs it a message it already knows how to give. Filed out of the #95 code review, where the shape was noted but deliberately not reshaped: that branch's charter was the list grammars, and this changes how the multipoint body is parsed.
## The visible symptom
The same fault reports two different ways depending on where it sits:
```text
MULTIPOINT(()) -> A multipoint must have at least one position. wrong
MULTIPOINT((0 0), ()) -> each vertex must be an 'x y' pair; got ''. right
MULTIPOINT((), (0 0)) -> each vertex must be an 'x y' pair; got ''. right
```
A requester who wrote one (empty) point is told they wrote none. The wording is not the problem: the message depends on the point's *position* rather than on the fault. GEOS draws the same distinction, which is decent confirmation these really are two different faults: `MULTIPOINT(())` gives "Expected number but encountered ')'" while `MULTIPOINT()` gives "Unexpected token: )".
## Cause
`flattened = body.replace("(", " ").replace(")", " ")` erases the parentheses, so `MULTIPOINT(())` and `MULTIPOINT()` both reach `_parse_xy_pairs` as whitespace. Its blank short-circuit then reports emptiness, which is correct for `POLYGON(())` (yielding "at least four vertices (a closed triangle); in ring 0 got 0") and wrong here. The evidence that a point was written is destroyed one line before anything checks for it.
## Proposed
Extract the per-vertex body of `_parse_xy_pairs` into a singular `_parse_xy_pair`, have the plural map it over its split (so `parse_polygon_wkt` is unchanged), and have `parse_multipoint_wkt` call the singular once per already-validated point instead of flattening and re-splitting. The empty-versus-absent distinction then survives in `points`, and the message becomes uniform without anyone writing a new one. Roughly 15 lines net.
This also collapses a structural finding from the same review: the multipoint path is currently a per-item `fullmatch` plus a `len({p.startswith("(") for p in points}) > 1` set-cardinality trick, a mechanism that exists nowhere else in the module, where the polygon sibling validates its whole list with one grammar. Parsing per point makes the two paths read the same way and removes the flatten step along with its "only balanced parentheses reach here" reasoning.
## What not to do
Do not tighten `_MULTIPOINT_POINT` to `\([^(),]+\)`. It routes `()` to the structural guard, but it also downgrades `MULTIPOINT((0 0), ())` from the specific message to the generic "malformed point list", losing a good message to gain consistency on a degenerate one.
Do not replace the two guards with a single pattern spanning the list. The bare branch puts `[^(),]+` next to `\s*`, which is the exact ambiguity that caused the denial of service fixed in #95; it would need its own linearity proof to be safe.
## Priority
Low. `MULTIPOINT(())` is a degenerate input, it still yields a 400, and nothing is corrupted. The refactor earns its keep as structural cleanup that happens to fix this, not as a fix for one bad message.
Contributor guide
No contributing guide indexed for this repository
Research direction
Read parse_multipoint_wkt and _parse_xy_pairs first, then trace parse_polygon_wkt to preserve its behavior. Done means multipoint parsing keeps empty and absent points distinct, emits the existing per-vertex error for empty points, and leaves polygon parsing unchanged; verify with the parser's existing tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend-api-design
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100