developmentseed / developmentseed/titiler-covjson

An out-of-range or malformed band inside an `expression` is an unhandled 500

Open
#107 0 comments 0 reactions 0 assignees View on GitHub
chore
Dominant language
Python
Stars
1
Forks
1
Avg merge
2d 1h
Merged PRs (30d)
12

Description

A band reference inside an `expression` is never checked against the dataset, so three ordinary client mistakes escape as unhandled exceptions and render as `500`. `_validate_band_indexes` (`src/titiler_covjson/factory.py`) already turns exactly these faults into actionable `400`s for `bidx` and `parameter-name`, and its docstring says expression band references are "handled where the expression is parsed" -- but only *duplicates* are.

## Measured

On the 2-band `cog_path` fixture (bands `b1`, `b2`), every route, no exception handler catches these:

| `expression` | outcome | raised by |
| --- | --- | --- |
| `b9` | `IndexError: band index 9 out of range (not in (1, 2))` | rasterio, at the read |
| `b0` | `IndexError: band index 0 out of range (not in (1, 2))` | rasterio, at the read |
| `b1+b2b` | `ValueError: invalid literal for int() with base 10: '2b'` | rio-tiler's `parse_expression` |

All four routes (`/bbox`, `/area`, `/position` with `POINT`, `/position` with `MULTIPOINT`) behave identically. `ValueError` and `IndexError` are not in titiler's `DEFAULT_STATUS_CODES`, so they fall through to the `Exception` catch-all and a deployment returns `500` with no actionable detail.

`b1+b2b` is worth separating from the other two: it dies inside `parse_expression` itself, before any read, so a fix that only range-checks that function's *output* leaves it a `500`.

`b0` also exposes a drift between the two selectors. `_BAND_NAME` (`src/titiler_covjson/dependencies.py`) rejects `b0` for `parameter-name` because it requires `[1-9][0-9]*`, so `parameter-name=b0` is a clean `400` while `expression=b0` crashes. Same rule, two answers.

## Fix

All four read paths already funnel through one guard, each calling `_validate_band_indexes(band_kwargs.get("indexes"), info)`:

```text
factory.py:500 _read_bounded_image (/bbox)
factory.py:595 _read_point (/position POINT)
factory.py:659 _read_multipoint (/position MULTIPOINT)
factory.py:746 _read_polygon_image (/area)
```

Widen that single guard rather than patching one path: pass the whole band selection so it can see `expression` as well as `indexes`, wrap the `parse_expression` call in `except (InvalidExpression, ValueError)` -> `BadRequestError`, and range-check the indexes it returns against `len(info.band_descriptions)`, reusing the existing out-of-range message.

Two things not to do:

- Do not apply the duplicate-index check to expression bands. `parse_expression` builds its result from a `set`, so `parse_expression("b1;b1") == (1,)` and a duplicate can never appear; `b1+b1` is a legitimate expression.
- Do not exclude the alpha band from the count. Unlike the read-time band resolution in #105, a caller may legitimately name an alpha band as `b4` in an expression, so `len(info.band_descriptions)` is the right denominator here and the two issues do not interact.

## Ordering constraint

`parse_expression` raises rio-tiler's own error on a degenerate expression, which is why `_selected_band_count` carries a `# Validate first:` comment: `_expression_band_names` must run before it, or rio-tiler's message wins over ours. The new guard runs *before* `_selected_band_count` on the `/bbox` path, so it has to keep that ordering or `test_bbox_blank_expression_block_names_its_position` regresses from our "Blank sub-expression" message to rio-tiler's "unexpected indent".

That constraint is temporary. #104 moves expression validation to the request edge, after which nothing downstream depends on the call order.

## Test

A four-route parametrized sweep over all three faults asserting `400` on every route, plus an in-range multi-band expression (`b1+b2`) that still serves, so the guard does not over-reject. The existing blank-block test is the ordering regression guard.

## Docs

`docs/08-bbox-endpoint-spec.md` Section 10 currently documents this `500` as intended: the `400` row needs "a band index out of range" widened to cover an expression, and the `404` paragraph's clause naming a bad expression band reference as a `500` has to go.

## Priority

Higher than #103, #104, and #105, all of which yield an actionable `400` today. This one is an unhandled exception and a misleading status code: a client mistake is reported as a server fault, which also means it pollutes `5xx` metrics and gets retried by a content delivery network. Found while planning #104.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/titiler_covjson/factory.py at the shared read guards around _read_bounded_image, _read_point, _read_multipoint, and _read_polygon_image; also inspect _validate_band_indexes and expression parsing. Add the parametrized four-route regression coverage described in the issue, preserve the blank-expression ordering test, and update Section 10 of docs/08-bbox-endpoint-spec.md. Done means malformed or out-of-range expression bands return 400 while b1+b2 still serves.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.