From 39dabc05195734e04ca7e8aba15b7e85f6ecda0d Mon Sep 17 00:00:00 2001 From: matthewberry Date: Wed, 4 Nov 2026 10:47:40 -0600 Subject: [PATCH 1/2] fix: reject out-of-range limit and offset; previous past the end goes to the last page limit and offset went into the SQL unchecked, so a negative or oversized value made Postgres fail and the client got a 500 with its error. Bound them on the Query parameters (limit 1 to 2^63-1, offset 0 to 2^63-1), so those get a 422 naming the parameter and no SQL runs. Past the end of the results, previous pointed at another empty page. Point it at the last page. Same fixes as CLEANDB-api#25 and #27. --- app/routers/data.py | 16 ++++++--- tests/test_pagination_bounds.py | 57 +++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 4 deletions(-) create mode 100644 tests/test_pagination_bounds.py diff --git a/app/routers/data.py b/app/routers/data.py index c8d1971..59a8484 100644 --- a/app/routers/data.py +++ b/app/routers/data.py @@ -12,6 +12,9 @@ from app.db.queries import get_filtered_data, get_total_count from app.models.query_params import OEDDataQueryParams, ResponseFormat +# Largest value Postgres accepts for LIMIT and OFFSET +MAX_BIGINT = 2**63 - 1 + router = APIRouter(tags=["Data"]) @@ -48,9 +51,11 @@ def parse_query_params( None, description="Columns to include in the response" ), limit: Optional[int] = Query( - None, description="Maximum number of records to return" + None, ge=1, le=MAX_BIGINT, description="Maximum number of records to return" + ), + offset: Optional[int] = Query( + 0, ge=0, le=MAX_BIGINT, description="Number of records to skip" ), - offset: Optional[int] = Query(0, description="Number of records to skip"), ) -> OEDDataQueryParams: """Parse and validate query parameters.""" try: @@ -240,11 +245,14 @@ async def get_data( } next_url = f"{base_url}?{urlencode(next_params, doseq=True)}" - # Calculate previous page link if not on first page + # Calculate previous page link if not on first page. Past the end, go + # back to the last page instead of another empty one. if current_offset > 0: prev_params = { **query_params, - "offset": max(0, current_offset - current_limit), + "offset": max( + 0, min(current_offset - current_limit, total_count - current_limit) + ), "limit": current_limit, } previous_url = f"{base_url}?{urlencode(prev_params, doseq=True)}" diff --git a/tests/test_pagination_bounds.py b/tests/test_pagination_bounds.py new file mode 100644 index 0000000..1647aef --- /dev/null +++ b/tests/test_pagination_bounds.py @@ -0,0 +1,57 @@ +import pytest + +from app.core.config import settings + +DATA = "/api/v1/data?ec=1.1.1.1" +TOO_BIG = 2**63 # one past Postgres bigint + +# Out-of-range values used to reach the SQL: negative or oversized ones made +# Postgres fail (a 500 carrying the Postgres error), and limit=0 returned no rows. +REJECTED = [ + ("limit", -1), + ("limit", 0), + ("limit", TOO_BIG), + ("offset", -5), + ("offset", TOO_BIG), +] + +ACCEPTED = [ + ("limit", 1), + ("offset", 0), + ("offset", TOO_BIG - 1), +] + + +@pytest.mark.parametrize("param, value", REJECTED) +def test_out_of_range_pagination_is_rejected(test_client, sql_capture, param, value): + response = test_client.get(f"{DATA}&{param}={value}") + + assert response.status_code == 422, response.text + assert [e["loc"] for e in response.json()["detail"]] == [["query", param]] + assert sql_capture == [] + + +@pytest.mark.parametrize("param, value", ACCEPTED) +def test_pagination_bounds_are_inclusive(test_client, sql_capture, param, value): + response = test_client.get(f"{DATA}&{param}={value}") + + assert response.status_code == 200, response.text + data_query = [q["query"] for q in sql_capture if "COUNT(*)" not in q["query"]][0] + assert f"{param.upper()} {value}" in data_query + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "mock_db", + [{"fetchval": lambda self, query, *args: 25, "fetch": lambda self, query, *args: []}], + indirect=True, +) +async def test_previous_past_the_end_goes_to_the_last_page(monkeypatch, test_client, mock_db): + """Past the end, `previous` used to point at another empty page.""" + monkeypatch.setattr(settings, "AUTO_PAGINATION_THRESHOLD", 10) + + data = test_client.get("/api/v1/data?offset=100&limit=10").json() + + assert data["data"] == [] + assert "offset=15" in data["previous"] + assert "next" not in data From 894a3c27f60326dc9ea20c14ceff6ce5d5d42a19 Mon Sep 17 00:00:00 2001 From: matthewberry Date: Wed, 4 Nov 2026 11:07:41 -0600 Subject: [PATCH 2/2] docs: previous past the end goes to the last limit records, not a page boundary --- app/routers/data.py | 3 ++- tests/test_pagination_bounds.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/routers/data.py b/app/routers/data.py index 59a8484..c865c71 100644 --- a/app/routers/data.py +++ b/app/routers/data.py @@ -246,7 +246,8 @@ async def get_data( next_url = f"{base_url}?{urlencode(next_params, doseq=True)}" # Calculate previous page link if not on first page. Past the end, go - # back to the last page instead of another empty one. + # back to the last `limit` records instead of another empty page. That + # isn't necessarily a page boundary: 25 records in pages of 10 gives 15. if current_offset > 0: prev_params = { **query_params, diff --git a/tests/test_pagination_bounds.py b/tests/test_pagination_bounds.py index 1647aef..bb8fb55 100644 --- a/tests/test_pagination_bounds.py +++ b/tests/test_pagination_bounds.py @@ -46,7 +46,7 @@ def test_pagination_bounds_are_inclusive(test_client, sql_capture, param, value) [{"fetchval": lambda self, query, *args: 25, "fetch": lambda self, query, *args: []}], indirect=True, ) -async def test_previous_past_the_end_goes_to_the_last_page(monkeypatch, test_client, mock_db): +async def test_previous_past_the_end_goes_to_the_last_records(monkeypatch, test_client, mock_db): """Past the end, `previous` used to point at another empty page.""" monkeypatch.setattr(settings, "AUTO_PAGINATION_THRESHOLD", 10)