developmentseed / developmentseed/titiler-covjson
Validate `expression` at the request edge, not at read depth
- Dominant language
- Python
- Stars
- 1
- Forks
- 1
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 12
Description
`expression` validity is a property of the request alone, but it is checked at read depth. The consequence is that the same malformed expression is diagnosed by whoever happens to get there first, which differs by route, and that an unopenable dataset shadows the check entirely.
## The visible symptom
Same input, four routes:
```text
expression=b1; ;b2
/bbox -> 400 Blank sub-expression: ... blank at position 1
/area -> 400 Blank sub-expression: ... blank at position 1
/position MULTI -> 400 Blank sub-expression: ... blank at position 1
/position POINT -> 400 Invalid expression syntax: unexpected indent <- rio-tiler's
```
`/position` with a single `POINT` needs no cell ceiling (it reads one pixel), so nothing resolves a band count before the read and the expression is only inspected afterwards, by which time `point()` has already raised.
That this is a race rather than a rule shows up in the neighboring case. A duplicate expression reports uniformly on all four routes:
```text
expression=b1;b1 -> 400 Duplicate expression: derived band names must be unique; (x4)
```
because `b1;b1` is syntactically valid to rio-tiler, so the read succeeds and our post-read check wins. A blank block is not valid, so rio-tiler wins. Depth decides who reports the fault, not intent.
The second symptom is that a check needing no dataset is nonetheless gated behind opening one:
```text
url=/no/such/file.tif & expression=b1; ;b2
/bbox -> 500
/position POINT -> 500
```
## Cause
`_expression_band_names` (`src/titiler_covjson/factory.py`) is the single definition of the rule, but it is *reached* at three different depths: pre-read on `/bbox` and `/area` (incidentally, via the cell ceiling's band count, added in #100), pre-read on a `MULTIPOINT` (via an explicit call kept for its side effect alone), and post-read on a single `POINT` (via `_resolve_read_bands`). One rule, three placements, and the deepest one loses.
## Proposed
Validate the expression at the request edge, in `CovJSONBandParams.__post_init__` (`src/titiler_covjson/dependencies.py`). That dependency already validates the band selection (mutual exclusivity of `bidx` / `expression` / `parameter-name`, and the `parameter-name` fold), it runs on every route, and it runs before the dataset is opened. Both symptoms close at once: the diagnosis stops depending on the route, and a malformed expression is a 400 rather than a 500 behind an unreadable `url`.
This means moving `_expression_band_names` from `factory.py` to `dependencies.py`. Verified import-safe: `dependencies.py` imports only `titiler_covjson.reduce`, and `factory.py` already imports `dependencies.py`, so there is no cycle. Three callers repoint (`_resolve_unread_bands`, `_resolve_read_bands`, `_read_multipoint`), and `_read_multipoint`'s pre-read guard becomes dead once the dependency has already validated.
Worth deciding as part of this: whether the derivation and the validation should stay one function. `_expression_band_names` returns names *and* raises, so guard sites call a query purely for its side effect. Splitting a `validate_expression` from the name derivation would make both honest, at the cost of walking the blocks twice.
## Test
Assert the same expression gives the same diagnosis on all four routes, which is the property that is actually wrong today. Add the unreadable-`url` case: a blank expression must be a 400 before the dataset open, using the established idiom (an unreadable `url` plus a malformed request must give 400, not the 500 a real dataset open would give).
## Priority
Low. Every case still yields a 400 with an actionable message on the route it was sent to, and nothing is mis-served; what is wrong is that the contract varies by verb. Filed out of a plumb review of #100, where the scope was the cell ceiling: the blank-block rule entered only because #100's hoisted band count misdiagnosed a neighboring input, and making expression validation uniform is a separate change with its own reasoning.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in src/titiler_covjson/dependencies.py at CovJSONBandParams.__post_init__, then trace _expression_band_names and its callers in src/titiler_covjson/factory.py. Add coverage for the malformed expression across /bbox, /area, /position MULTI, and /position POINT, plus an unreadable URL case. Done means malformed requests receive the same 400 diagnosis before dataset opening.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend-api-design
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100