diff --git a/app/db/queries.py b/app/db/queries.py index 03cb30b..9cf96c4 100644 --- a/app/db/queries.py +++ b/app/db/queries.py @@ -1,7 +1,5 @@ from typing import Any, Dict, List, Tuple -import rich - from app.db.database import Database from app.models.oed_data import OEDColumn from app.models.query_params import OEDDataQueryParams @@ -140,7 +138,6 @@ async def get_filtered_data( # Execute the query records = await db.fetch(query, *query_args) - rich.print(f"{records=}") return records diff --git a/app/main.py b/app/main.py index 6ae0181..bb46b4f 100644 --- a/app/main.py +++ b/app/main.py @@ -46,10 +46,11 @@ async def lifespan(app: FastAPI): ### Technical Note: Automatic Pagination -When a query would return more than {settings.AUTO_PAGINATION_THRESHOLD} records and no explicit limit is -provided, the API will automatically paginate results to return {settings.AUTO_PAGINATION_THRESHOLD} records -at a time. The response will include pagination metadata with links to navigate -to next and previous pages. +When a query would return more than {settings.AUTO_PAGINATION_THRESHOLD} records and no explicit limit, or one of +at least {settings.AUTO_PAGINATION_THRESHOLD}, is provided, the API will automatically paginate results to return +{settings.AUTO_PAGINATION_THRESHOLD} records at a time. A JSON response will include pagination metadata with links to +navigate to next and previous pages. A CSV response carries the same links in a `Link` header, and the total +number of matching records in an `X-Total-Count` header. ## Data Download diff --git a/app/routers/data.py b/app/routers/data.py index 4e7705d..c8d1971 100644 --- a/app/routers/data.py +++ b/app/routers/data.py @@ -94,7 +94,8 @@ async def get_data( The response format can be either JSON (default) or CSV. Results are automatically paginated when they exceed the configured threshold, unless -an explicit limit is provided. +an explicit limit below the threshold is provided. CSV responses carry the pagination +links in a `Link` header and the total in an `X-Total-Count` header. ### URL examples @@ -192,8 +193,13 @@ async def get_data( # Get total count for the query (without pagination) total_count = await get_total_count(db, params) - # Apply automatic pagination if results exceed threshold and no explicit limit provided - if total_count > settings.AUTO_PAGINATION_THRESHOLD and params.limit is None: + # Apply automatic pagination if results exceed threshold and no explicit limit, + # or one at least as large, was provided. Without the cap one request could load + # the whole table into memory. ">=" because the `next` links below carry + # limit=threshold, and those pages need links of their own. + if total_count > settings.AUTO_PAGINATION_THRESHOLD and ( + params.limit is None or params.limit >= settings.AUTO_PAGINATION_THRESHOLD + ): params.auto_paginated = True params.limit = settings.AUTO_PAGINATION_THRESHOLD logger.info( @@ -203,6 +209,46 @@ async def get_data( # Get data from database (now with potential auto-pagination applied) data = await get_filtered_data(db, params) + # Pagination links, for both response formats + next_url = None + previous_url = None + if params.auto_paginated and request: + base_url = str(request.url).split("?")[0] + + # Prepare query parameters for pagination links + # For Pydantic v2 compatibility + query_params = { + k: v + for k, v in params.model_dump().items() + if k not in ["auto_paginated", "offset", "limit"] + and v is not None + } + + # Set format explicitly if it was provided + if params.format != ResponseFormat.JSON: + query_params["format"] = params.format + + current_offset = params.offset or 0 + current_limit = params.limit or total_count + + # Calculate next page link if there are more records + if current_offset + current_limit < total_count: + next_params = { + **query_params, + "offset": current_offset + current_limit, + "limit": current_limit, + } + next_url = f"{base_url}?{urlencode(next_params, doseq=True)}" + + # Calculate previous page link if not on first page + if current_offset > 0: + prev_params = { + **query_params, + "offset": max(0, current_offset - current_limit), + "limit": current_limit, + } + previous_url = f"{base_url}?{urlencode(prev_params, doseq=True)}" + # Handle response format if params.format == ResponseFormat.CSV: # Create CSV response @@ -224,12 +270,25 @@ async def get_data( for row in data: writer.writerow(row) + # A CSV body can't carry pagination metadata, so it goes in headers + headers = { + "Content-Disposition": "attachment; filename=oed_data.csv", + "X-Total-Count": str(total_count), + } + links = [] + if next_url: + links.append(f'<{next_url}>; rel="next"') + if previous_url: + links.append(f'<{previous_url}>; rel="prev"') + if links: + headers["Link"] = ", ".join(links) + # Return streaming response output.seek(0) return StreamingResponse( iter([output.getvalue()]), media_type="text/csv", - headers={"Content-Disposition": "attachment; filename=oed_data.csv"}, + headers=headers, ) else: # JSON response with enhanced pagination info @@ -240,54 +299,13 @@ async def get_data( "data": data, } - # Add pagination links if automatic pagination was applied if params.auto_paginated: # Add flag indicating automatic pagination was applied response["auto_paginated"] = True - - if request: - base_url = str(request.url).split("?")[0] - - # Prepare query parameters for pagination links - # For Pydantic v2 compatibility - query_params = { - k: v - for k, v in params.model_dump().items() - if k not in ["auto_paginated", "offset", "limit"] - and v is not None - } - - # Set format explicitly if it was provided - if params.format != ResponseFormat.JSON: - query_params["format"] = params.format - - # Calculate next page link if there are more records - current_offset = params.offset or 0 - current_limit = params.limit or total_count - if current_offset + current_limit < total_count: - next_offset = current_offset + current_limit - next_params = { - **query_params, - "offset": next_offset, - "limit": current_limit, - } - response["next"] = ( - f"{base_url}?{urlencode(next_params, doseq=True)}" - ) - - # Calculate previous page link if not on first page - current_offset = params.offset or 0 - current_limit = params.limit or total_count - if current_offset > 0: - prev_offset = max(0, current_offset - current_limit) - prev_params = { - **query_params, - "offset": prev_offset, - "limit": current_limit, - } - response["previous"] = ( - f"{base_url}?{urlencode(prev_params, doseq=True)}" - ) + if next_url: + response["next"] = next_url + if previous_url: + response["previous"] = previous_url return response diff --git a/tests/test_auto_pagination.py b/tests/test_auto_pagination.py index bebd3be..43cb997 100644 --- a/tests/test_auto_pagination.py +++ b/tests/test_auto_pagination.py @@ -1,3 +1,5 @@ +import re + import pytest from fastapi.testclient import TestClient @@ -113,8 +115,108 @@ async def test_auto_pagination_not_needed(monkeypatch, test_client, mock_db): @pytest.mark.asyncio -async def test_pagination_navigation(monkeypatch): - """Test navigation through paginated results.""" - # Skip this test for now - we'll address it separately - # This test requires more complex mocking of the request/response cycle - pytest.skip("This test will be implemented in a separate PR") \ No newline at end of file +@pytest.mark.parametrize( + "mock_db", + [ + { + "fetchval": lambda self, query, *args: 25, + "fetch": lambda self, query, *args: [{"id": i, "ec": f"1.1.1.{i}", "organism": f"Test Organism {i}"} for i in range(10)] + } + ], + indirect=True +) +async def test_pagination_navigation(monkeypatch, test_client, mock_db): + """Test that following `next` reaches the last page. + + Every page after the first is requested with an explicit limit equal to the + threshold, so it must still be treated as paginated and carry its own links.""" + monkeypatch.setattr(settings, "AUTO_PAGINATION_THRESHOLD", 10) + + offsets = [] + url = "/api/v1/data" + while url: + data = test_client.get(url).json() + offsets.append(data["offset"]) + url = data.get("next", "").replace("http://testserver", "") + + assert offsets == [0, 10, 20] + assert "offset=10" in data["previous"] + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "mock_db", + [ + { + "fetchval": lambda self, query, *args: 100, + "fetch": lambda self, query, *args: [{"id": i, "ec": f"1.1.1.{i}", "organism": f"Test Organism {i}"} for i in range(10)] + } + ], + indirect=True +) +async def test_explicit_limit_above_threshold_is_capped(monkeypatch, test_client, mock_db, sql_capture): + """Test that an explicit limit larger than the threshold is reduced to the threshold. + + Without the cap, one request can load the whole table into memory.""" + monkeypatch.setattr(settings, "AUTO_PAGINATION_THRESHOLD", 10) + + response = test_client.get("/api/v1/data?limit=1000") + + assert response.status_code == 200 + data = response.json() + + # The query itself is limited, not just the response + data_query = [q["query"] for q in sql_capture if "COUNT(*)" not in q["query"]][0] + assert re.search(r"LIMIT 10\b", data_query) + + assert data["auto_paginated"] is True + assert data["limit"] == 10 + assert data["total"] == 100 + assert "offset=10" in data["next"] + assert "limit=10" in data["next"] + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "mock_db", + [ + { + "fetchval": lambda self, query, *args: 100, + "fetch": lambda self, query, *args: [{"id": i, "ec": "1.1.1.1", "organism": "Distinctive organism"} for i in range(5)] + } + ], + indirect=True +) +async def test_results_are_not_printed(test_client, mock_db, capsys): + """Test that query results are not written to stdout. + + Printing every record of a large result set cost far more memory than the records.""" + response = test_client.get("/api/v1/data?limit=5") + + assert response.status_code == 200 + assert "Distinctive organism" not in capsys.readouterr().out + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "mock_db", + [ + { + "fetchval": lambda self, query, *args: 100, + "fetch": lambda self, query, *args: [{"id": i, "ec": f"1.1.1.{i}", "organism": f"Test Organism {i}"} for i in range(10)] + } + ], + indirect=True +) +async def test_capped_csv_says_there_is_more(monkeypatch, test_client, mock_db): + """Test that a CSV response cut off by the cap says so in its headers. + + A CSV body has nowhere to carry pagination metadata.""" + monkeypatch.setattr(settings, "AUTO_PAGINATION_THRESHOLD", 10) + + response = test_client.get("/api/v1/data?format=csv&limit=1000") + + assert response.status_code == 200 + assert response.headers["x-total-count"] == "100" + assert 'rel="next"' in response.headers["link"] + assert "offset=10" in response.headers["link"] + assert "format=csv" in response.headers["link"]