From c0a666615cb297dcfbe0f4512cca22523aaf73c7 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Mon, 20 Jul 2026 16:01:32 +1000 Subject: [PATCH 1/2] fix(api): return 404/400 for malformed request input instead of 500 Non-numeric execution and group ids now return 404 instead of raising DataError or ValueError on the raw int cast. Malformed or non-object facets JSON on the datasets list route returns 400 instead of an unhandled JSONDecodeError or AttributeError. The AFT routes no longer copy internal exception text into the response body, they log the detail and return a generic 500 message. Fixes the sanitize_float_value docstring, which claimed NaN and infinity became 0.0 when the code has always returned None. Also drops the warning log on every non-float series value, since None and int are legitimate series values, not something to warn about. --- backend/src/ref_backend/api/routes/aft.py | 11 ++++++---- .../src/ref_backend/api/routes/datasets.py | 11 ++++++++-- .../src/ref_backend/api/routes/executions.py | 21 +++++++++++++++---- backend/src/ref_backend/core/json_utils.py | 18 +++++----------- .../test_api/test_routes/test_datasets.py | 21 +++++++++++++++++++ .../test_api/test_routes/test_executions.py | 19 +++++++++++++++++ backend/tests/test_core/test_json_utils.py | 6 ++++++ changelog/+api-input-handling.fix.md | 1 + 8 files changed, 85 insertions(+), 23 deletions(-) create mode 100644 changelog/+api-input-handling.fix.md diff --git a/backend/src/ref_backend/api/routes/aft.py b/backend/src/ref_backend/api/routes/aft.py index 9f79a6b..054f595 100644 --- a/backend/src/ref_backend/api/routes/aft.py +++ b/backend/src/ref_backend/api/routes/aft.py @@ -1,4 +1,5 @@ from fastapi import APIRouter, HTTPException +from loguru import logger from ref_backend.core.aft import get_aft_diagnostic_by_id, get_aft_diagnostics_index from ref_backend.models import AFTDiagnosticDetail, AFTDiagnosticSummary @@ -13,8 +14,9 @@ async def list_aft_diagnostics() -> list[AFTDiagnosticSummary]: """ try: return get_aft_diagnostics_index() - except ValueError as e: - raise HTTPException(status_code=500, detail=str(e)) from e + except ValueError: + logger.exception("Failed to load AFT diagnostics") + raise HTTPException(status_code=500, detail="Failed to load AFT diagnostics") from None @router.get("/{aft_id}", response_model=AFTDiagnosticDetail) @@ -27,5 +29,6 @@ async def get_aft_diagnostic(aft_id: str) -> AFTDiagnosticDetail: if diagnostic is None: raise HTTPException(status_code=404, detail="AFT diagnostic not found") return diagnostic - except ValueError as e: - raise HTTPException(status_code=500, detail=str(e)) from e + except ValueError: + logger.exception("Failed to load AFT diagnostics") + raise HTTPException(status_code=500, detail="Failed to load AFT diagnostics") from None diff --git a/backend/src/ref_backend/api/routes/datasets.py b/backend/src/ref_backend/api/routes/datasets.py index 1797acb..d1b3f3e 100644 --- a/backend/src/ref_backend/api/routes/datasets.py +++ b/backend/src/ref_backend/api/routes/datasets.py @@ -38,7 +38,12 @@ async def _list( # noqa: PLR0913 dataset_query = dataset_query.filter(models.Dataset.dataset_type == dataset_type.upper()) if facets: - facet_filters = json.loads(facets) + try: + facet_filters = json.loads(facets) + except json.JSONDecodeError: + raise HTTPException(status_code=400, detail="facets must be a JSON object") from None + if not isinstance(facet_filters, dict): + raise HTTPException(status_code=400, detail="facets must be a JSON object") for dataset_type_model in models.Dataset.__subclasses__(): if dataset_type_model.__mapper_args__["polymorphic_identity"].value == dataset_type: for key, value in facet_filters.items(): @@ -46,7 +51,9 @@ async def _list( # noqa: PLR0913 dataset_query = dataset_query.filter(getattr(dataset_type_model, key) == value) break elif facets: - raise ValueError("Cannot filter using facets if a source type is not specified") + raise HTTPException( + status_code=400, detail="Cannot filter using facets if a source type is not specified" + ) total_count = dataset_query.count() datasets = dataset_query.offset(offset).limit(limit) diff --git a/backend/src/ref_backend/api/routes/executions.py b/backend/src/ref_backend/api/routes/executions.py index ab1cfe9..d1f04cf 100644 --- a/backend/src/ref_backend/api/routes/executions.py +++ b/backend/src/ref_backend/api/routes/executions.py @@ -41,6 +41,13 @@ router = APIRouter(prefix="/executions", tags=["executions"]) +def _parse_int_id(value: str, resource: str) -> int: + try: + return int(value) + except ValueError: + raise HTTPException(status_code=404, detail=f"{resource} not found") from None + + @router.get("/statistics") async def get_execution_statistics(app_context: AppContextDep) -> ExecutionStats: """ @@ -200,7 +207,9 @@ async def get(app_context: AppContextDep, group_id: str) -> ExecutionGroup: """ Inspect a specific execution """ - execution_group = app_context.session.query(models.ExecutionGroup).get(group_id) + execution_group = app_context.session.query(models.ExecutionGroup).get( + _parse_int_id(group_id, "Execution group") + ) if not execution_group: raise HTTPException(status_code=404, detail="Execution not found") @@ -208,19 +217,23 @@ async def get(app_context: AppContextDep, group_id: str) -> ExecutionGroup: async def _get_execution(group_id: str, execution_id: str | None, session: Session) -> models.Execution: + group_id_int = _parse_int_id(group_id, "Execution group") + if execution_id is not None: - execution: models.Execution | None = session.query(models.Execution).get(execution_id) + execution: models.Execution | None = session.query(models.Execution).get( + _parse_int_id(execution_id, "Execution") + ) else: # Fetch only the latest execution for the group without loading the full collection execution = ( session.query(models.Execution) - .filter(models.Execution.execution_group_id == int(group_id)) + .filter(models.Execution.execution_group_id == group_id_int) .order_by(models.Execution.id.desc()) .limit(1) .one_or_none() ) - if not execution or not execution.execution_group_id == int(group_id): + if not execution or not execution.execution_group_id == group_id_int: raise HTTPException(status_code=404, detail="Result not found") return execution diff --git a/backend/src/ref_backend/core/json_utils.py b/backend/src/ref_backend/core/json_utils.py index f2692c4..7e48f48 100644 --- a/backend/src/ref_backend/core/json_utils.py +++ b/backend/src/ref_backend/core/json_utils.py @@ -5,8 +5,6 @@ import math from typing import Any -from loguru import logger - def sanitize_float_value(value: float | Any) -> float | Any: """ @@ -17,18 +15,12 @@ def sanitize_float_value(value: float | Any) -> float | Any: Returns ------- - - 0.0 if the value is NaN - - 0.0 if the value is infinity (positive or negative) + - None if the value is NaN + - None if the value is infinity (positive or negative) - The original value if it's a valid float or not a float """ - if isinstance(value, float): - if math.isnan(value): - # The None values will be converted to null in JSON - return None - if math.isinf(value): - return None - else: - logger.warning(f"Non-float value encountered during sanitization: {value}") + if isinstance(value, float) and (math.isnan(value) or math.isinf(value)): + return None return value @@ -41,7 +33,7 @@ def sanitize_float_list(values: list[float | Any]) -> list[float | Any]: Returns ------- - List with NaN and infinity values replaced with 0.0 + List with NaN and infinity values replaced with None """ return [sanitize_float_value(value) for value in values] diff --git a/backend/tests/test_api/test_routes/test_datasets.py b/backend/tests/test_api/test_routes/test_datasets.py index 2732a4f..5f2c650 100644 --- a/backend/tests/test_api/test_routes/test_datasets.py +++ b/backend/tests/test_api/test_routes/test_datasets.py @@ -61,3 +61,24 @@ def test_dataset_executions(client: TestClient, settings): r = client.get(f"{settings.API_V1_STR}/datasets/{dataset_id}/executions") assert r.status_code == 200 + + +def test_dataset_list_invalid_facets_json(client: TestClient, settings): + """Test that malformed facets JSON returns 400 instead of a 500.""" + r = client.get(f"{settings.API_V1_STR}/datasets/?dataset_type=cmip6&facets=notjson") + + assert r.status_code == 400 + + +def test_dataset_list_non_object_facets_json(client: TestClient, settings): + """Test that facets JSON that is not an object returns 400 instead of a 500.""" + r = client.get(f"{settings.API_V1_STR}/datasets/?dataset_type=cmip6&facets=%5B1%2C2%5D") + + assert r.status_code == 400 + + +def test_dataset_list_facets_without_dataset_type(client: TestClient, settings): + """Test that facets without a dataset_type returns 400 instead of a 500.""" + r = client.get(f'{settings.API_V1_STR}/datasets/?dataset_type=&facets={{"a":"b"}}') + + assert r.status_code == 400 diff --git a/backend/tests/test_api/test_routes/test_executions.py b/backend/tests/test_api/test_routes/test_executions.py index 2e9a251..fda2506 100644 --- a/backend/tests/test_api/test_routes/test_executions.py +++ b/backend/tests/test_api/test_routes/test_executions.py @@ -371,6 +371,25 @@ def test_execution_404_invalid_id(client: TestClient, settings) -> None: assert r.status_code == 404 +def test_execution_404_non_numeric_group_id(client: TestClient, settings) -> None: + """Test that a non-numeric group id returns 404 instead of a 500.""" + r = client.get(f"{settings.API_V1_STR}/executions/not-a-number") + assert r.status_code == 404 + + +def test_execution_404_non_numeric_group_id_on_execution_route(client: TestClient, settings) -> None: + """Test that a non-numeric group id on the execution sub-route returns 404.""" + r = client.get(f"{settings.API_V1_STR}/executions/not-a-number/execution") + assert r.status_code == 404 + + +def test_execution_404_non_numeric_execution_id(client: TestClient, settings) -> None: + """Test that a non-numeric execution_id query param returns 404.""" + group_id = get_execution_group_id(client, settings) + r = client.get(f"{settings.API_V1_STR}/executions/{group_id}/execution?execution_id=abc") + assert r.status_code == 404 + + def test_execution_datasets(client: TestClient, settings) -> None: """Test getting datasets for an execution group.""" # Get an execution group ID from the list diff --git a/backend/tests/test_core/test_json_utils.py b/backend/tests/test_core/test_json_utils.py index ebb5c3d..bc35e83 100644 --- a/backend/tests/test_core/test_json_utils.py +++ b/backend/tests/test_core/test_json_utils.py @@ -68,6 +68,12 @@ def test_all_nan_values(self): result = sanitize_float_list(values) assert result == [None, None, None] + def test_none_and_int_values_pass_through(self): + """Test that None and int values in a series pass through unchanged.""" + values = [1.0, None, 2] + result = sanitize_float_list(values) + assert result == [1.0, None, 2] + class TestSanitizeDictValues: """Test the sanitize_dict_values function.""" diff --git a/changelog/+api-input-handling.fix.md b/changelog/+api-input-handling.fix.md new file mode 100644 index 0000000..65da589 --- /dev/null +++ b/changelog/+api-input-handling.fix.md @@ -0,0 +1 @@ +Returns 404/400 for malformed ids and facet filters instead of a 500, and stops leaking internal error detail from the AFT routes. From 7335252427fc414d359236acb5b8424ffb0e2750 Mon Sep 17 00:00:00 2001 From: Jared Lewis Date: Mon, 20 Jul 2026 16:04:47 +1000 Subject: [PATCH 2/2] docs(changelog): number the fragment for PR #44 --- changelog/{+api-input-handling.fix.md => 44.fix.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog/{+api-input-handling.fix.md => 44.fix.md} (100%) diff --git a/changelog/+api-input-handling.fix.md b/changelog/44.fix.md similarity index 100% rename from changelog/+api-input-handling.fix.md rename to changelog/44.fix.md