[matthewberry]

Ports moleculemaker/CLEANDB-api#25 (bounds) and the "previous past the end" part of moleculemaker/CLEANDB-api#27 to /api/v1/data.

Problem

limit and offset reach the SQL unchecked. From production:

GET /api/v1/data?limit=-1&columns=ec
500 {"detail":"Error retrieving data: LIMIT must not be negative"}

A negative offset fails the same way, and Postgres rejects values above the bigint maximum (2^63 - 1) with "bigint out of range" (seen on CLEANDB-api's Postgres 16; not re-checked here). limit=0 returned no rows.

previous past the end points at another empty page. From production, with 35,753 records:

GET /api/v1/data?offset=100000&limit=5000&columns=ec
"previous": "…?format=json&columns=ec&offset=95000&limit=5000"   (also empty)

Fix

• limit is now 1 to 2^63 - 1 and offset 0 to 2^63 - 1, as Query(ge=…, le=…) bounds, so out-of-range values get a 422 naming the parameter and no SQL runs.
• previous uses max(0, min(offset - limit, total - limit)), so past the end it goes to the last limit records (or offset=0 when there are fewer than limit results). That isn't necessarily a page boundary: with 25 records and pages of 10 it's offset=15, not 20. Every record is still reachable. Within the results the link is unchanged.

The part of CLEANDB-api#27 about filters being dropped from links doesn't apply: oed-api's model field names match its query parameter names.

Tests

New tests/test_pagination_bounds.py, modeled on CLEANDB-api's:

• limit of -1, 0 or 2^63, and offset of -5 or 2^63, each get a 422 whose only error is on that parameter, with no query sent to the database.
• limit=1, offset=0 and offset=2^63 - 1 are accepted and reach the SQL unchanged.
• With 25 records and a page size of 10, offset=100 gets previous at offset=15 and no next.

The six rejection and previous cases fail on main. Full suite: 32 passed (23 before, against a throwaway Postgres for the app's startup connection, as in #8).

Not changed here

Requests above the page size are still capped at 5,000 for both formats, as #8 left them. A follow-up PR, stacked on this one, changes that to match CLEANDB-api#20.

[matthewberry]

Looks good. Reproduced: full suite 32 passed, and against main's app/, 6 of the 9 new tests in tests/test_pagination_bounds.py fail.

Nit on wording: "previous past the end goes to the last page" is really "the last limit records". With 25 records and pages of 10, previous goes to offset=15, not the page boundary at 20. Every row is still reachable, so only the PR text needs changing, unless lining up with page boundaries matters.

Couldn't reproduce, and it predates this PR: the data query has no ORDER BY, so limit/offset pages aren't guaranteed to be consistent between requests. With a 35k-row table it's unlikely to show up.

[matthewberry]

Agreed on the wording. Fixed in 894a3c2: the code comment, the test name (test_previous_past_the_end_goes_to_the_last_records), the PR title and the PR body now say it goes to the last limit records, with the 25-records-in-pages-of-10 example giving offset=15. I left the behavior alone; it matches CLEANDB-api#27, and every row stays reachable.

On ORDER BY: agreed, it predates this PR, and #11's 400 message now tells callers to page with limit/offset, which makes it matter a bit more. oed.oed_data has no key column (see db_migration/oed_03_data.sql), so there's nothing unique to sort by without adding one. I'd rather track that as its own issue than fold it into these PRs.