From 180c5bf3b06fe35f9970c77d66062a5b7719a89d Mon Sep 17 00:00:00 2001 From: Philippe Parage <69145356+pparage@users.noreply.github.com> Date: Thu, 6 Aug 2026 13:37:36 +0200 Subject: [PATCH] fix(api): declare the real v1 error envelope and the SSE content type (#116) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two response contracts in the committed spec did not match what the app returns. Since that spec bootstraps the Kong gateway and is what clients would be generated from, both produce clients that mis-parse real responses. 422: app/core/errors installs a RequestValidationError handler returning the Range42 envelope (error/message/code/details/trace_id/timestamp), but every v1 operation advertised FastAPI's default {"detail": [...]}. Added ErrorEnvelope mirroring _envelope() and declared it once on the aggregate v1 router, so all 48 operations inherit it rather than each route repeating itself. SSE: /v1/deployments/{id}/events returns EventSourceResponse, so its body is text/event-stream. It advertised application/json, which would have a generated consumer awaiting a body that never completes instead of opening a stream. The first test asserts a real 422 body and the declared schema have the same keys — asserting on the spec alone would let both drift together. v0 still advertises the default 422; those routes do not go through the v1 handlers and are decom-track anyway, so they are deliberately left. --- app/routes/v1/__init__.py | 10 +- app/routes/v1/deployments/events.py | 11 +- app/schemas/v1/common.py | 17 ++ openapi.json | 207 ++++++++++++++++----- tests/routes/test_v1_response_contracts.py | 95 ++++++++++ 5 files changed, 291 insertions(+), 49 deletions(-) create mode 100644 tests/routes/test_v1_response_contracts.py diff --git a/app/routes/v1/__init__.py b/app/routes/v1/__init__.py index 2f5f668..5534023 100644 --- a/app/routes/v1/__init__.py +++ b/app/routes/v1/__init__.py @@ -7,8 +7,16 @@ from app.routes.v1.proxmox import router as proxmox_router from app.routes.v1.admin import router as admin_router from app.routes.v1.health import router as health_router +from app.schemas.v1.common import ErrorEnvelope -router = APIRouter(prefix="/v1") +# Every v1 error goes through the handlers in app/core/errors, which return +# the Range42 envelope — not FastAPI's default {"detail": [...]}. Declaring it +# once here keeps generated clients honest for all of /v1 (#116). +_ERROR_RESPONSES = { + 422: {"model": ErrorEnvelope, "description": "Validation Error"}, +} + +router = APIRouter(prefix="/v1", responses=_ERROR_RESPONSES) router.include_router(catalog_router) router.include_router(projects_router) router.include_router(deployments_router) diff --git a/app/routes/v1/deployments/events.py b/app/routes/v1/deployments/events.py index a6e6994..25eabc4 100644 --- a/app/routes/v1/deployments/events.py +++ b/app/routes/v1/deployments/events.py @@ -44,7 +44,16 @@ def _filter_event(ev: dict, *, team: int | None, stage: str | None, return True -@router.get("/{deployment_id}/events") +@router.get( + "/{deployment_id}/events", + response_class=EventSourceResponse, + responses={ + 200: { + "description": "Server-sent event stream of deployment events", + "content": {"text/event-stream": {"schema": {"type": "string"}}}, + }, + }, +) async def events_stream(deployment_id: str, team: int | None = Query(None), stage: str | None = Query(None), diff --git a/app/schemas/v1/common.py b/app/schemas/v1/common.py index 6d387fb..9f28e6d 100644 --- a/app/schemas/v1/common.py +++ b/app/schemas/v1/common.py @@ -19,3 +19,20 @@ class ErrorDetail(BaseModel): field: str reason: str hint: str | None = None + + +class ErrorEnvelope(BaseModel): + """What every v1 error actually returns. + + Mirrors ``app.core.errors._envelope``. Declared so the generated spec + stops advertising FastAPI's default ``{"detail": [...]}`` for 422s — + clients built from the committed spec were deserialising the wrong + shape for every validation failure. + """ + + error: str + message: str + code: str + details: list[ErrorDetail] = [] + trace_id: str + timestamp: str diff --git a/openapi.json b/openapi.json index 46e4d4d..ee455fd 100644 --- a/openapi.json +++ b/openapi.json @@ -2128,7 +2128,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2167,7 +2167,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2202,7 +2202,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2244,7 +2244,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2345,7 +2345,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2396,7 +2396,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2449,7 +2449,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2488,7 +2488,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2540,7 +2540,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2576,7 +2576,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2628,7 +2628,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2670,7 +2670,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2723,7 +2723,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2762,7 +2762,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2804,7 +2804,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2854,7 +2854,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2896,7 +2896,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -2946,7 +2946,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3065,10 +3065,12 @@ ], "responses": { "200": { - "description": "Successful Response", + "description": "Server-sent event stream of deployment events", "content": { - "application/json": { - "schema": {} + "text/event-stream": { + "schema": { + "type": "string" + } } } }, @@ -3077,7 +3079,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3119,7 +3121,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3159,7 +3161,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3210,7 +3212,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3262,7 +3264,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3314,7 +3316,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3356,7 +3358,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3398,7 +3400,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3440,7 +3442,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3493,7 +3495,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3532,7 +3534,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3567,7 +3569,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3609,7 +3611,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3651,7 +3653,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3717,7 +3719,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3791,7 +3793,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3866,7 +3868,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3917,7 +3919,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -3983,7 +3985,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -4025,7 +4027,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -4087,7 +4089,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -4152,7 +4154,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -4225,7 +4227,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -4300,7 +4302,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -4375,7 +4377,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -4400,6 +4402,16 @@ } } } + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorEnvelope" + } + } + } } } } @@ -4422,6 +4434,16 @@ } } } + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorEnvelope" + } + } + } } } }, @@ -4458,7 +4480,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorEnvelope" } } } @@ -4478,6 +4500,16 @@ "schema": {} } } + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorEnvelope" + } + } + } } } } @@ -4494,6 +4526,16 @@ "schema": {} } } + }, + "422": { + "description": "Validation Error", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorEnvelope" + } + } + } } } } @@ -5059,6 +5101,77 @@ ], "title": "DownloadUrlIn" }, + "ErrorDetail": { + "properties": { + "field": { + "type": "string", + "title": "Field" + }, + "reason": { + "type": "string", + "title": "Reason" + }, + "hint": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "title": "Hint" + } + }, + "type": "object", + "required": [ + "field", + "reason" + ], + "title": "ErrorDetail" + }, + "ErrorEnvelope": { + "properties": { + "error": { + "type": "string", + "title": "Error" + }, + "message": { + "type": "string", + "title": "Message" + }, + "code": { + "type": "string", + "title": "Code" + }, + "details": { + "items": { + "$ref": "#/components/schemas/ErrorDetail" + }, + "type": "array", + "title": "Details", + "default": [] + }, + "trace_id": { + "type": "string", + "title": "Trace Id" + }, + "timestamp": { + "type": "string", + "title": "Timestamp" + } + }, + "type": "object", + "required": [ + "error", + "message", + "code", + "trace_id", + "timestamp" + ], + "title": "ErrorEnvelope", + "description": "What every v1 error actually returns.\n\nMirrors ``app.core.errors._envelope``. Declared so the generated spec\nstops advertising FastAPI's default ``{\"detail\": [...]}`` for 422s \u2014\nclients built from the committed spec were deserialising the wrong\nshape for every validation failure." + }, "FirewallAliasAddItemReply": { "properties": { "action": { diff --git a/tests/routes/test_v1_response_contracts.py b/tests/routes/test_v1_response_contracts.py new file mode 100644 index 0000000..6f695a3 --- /dev/null +++ b/tests/routes/test_v1_response_contracts.py @@ -0,0 +1,95 @@ +"""The spec must describe what v1 actually returns (#116). + +The committed openapi.json bootstraps the Kong gateway and is what clients +would be generated from, so a wrong declaration is not cosmetic: it produces +clients that mis-parse real responses. +""" +import json + +import pytest +from httpx import ASGITransport, AsyncClient + +ENVELOPE_KEYS = {"error", "message", "code", "details", "trace_id", "timestamp"} + + +async def _boot(tmp_path, monkeypatch): + monkeypatch.setenv("RANGE42_DB_URL", f"sqlite+aiosqlite:///{tmp_path / 't.db'}") + monkeypatch.setenv("RANGE42_WORKSPACE_ROOT", str(tmp_path)) + from importlib import reload + + from app.core import config as cfg + reload(cfg) + import app.core.db as dbmod + reload(dbmod) + from app.core.models import Base + + async with dbmod.get_engine().begin() as conn: + await conn.run_sync(Base.metadata.create_all) + + from app.main import create_app + return create_app() + + +@pytest.mark.asyncio +async def test_a_real_422_matches_the_declared_schema(tmp_path, monkeypatch): + """Anchor: the runtime body and the declaration must agree. + + Asserting only on the spec would let both drift together. + """ + app = await _boot(tmp_path, monkeypatch) + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + r = await c.post("/v1/projects/", json={}) # missing required fields + spec = (await c.get("/docs/openapi.json")).json() + + assert r.status_code == 422 + assert set(r.json()) == ENVELOPE_KEYS, r.text + + declared = (spec["paths"]["/v1/projects/"]["post"]["responses"]["422"] + ["content"]["application/json"]["schema"]["$ref"]) + name = declared.rsplit("/", 1)[-1] + props = set(spec["components"]["schemas"][name]["properties"]) + assert props == ENVELOPE_KEYS, f"{name} does not describe the real body" + + +@pytest.mark.asyncio +async def test_no_v1_operation_still_declares_the_fastapi_default( + tmp_path, monkeypatch, +): + app = await _boot(tmp_path, monkeypatch) + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + spec = (await c.get("/docs/openapi.json")).json() + + offenders = [] + for path, ops in spec["paths"].items(): + if not path.startswith("/v1/"): + continue + for method, op in ops.items(): + if not isinstance(op, dict): + continue + resp = (op.get("responses") or {}).get("422") + if not resp: + continue + ref = json.dumps(resp) + if "HTTPValidationError" in ref: + offenders.append(f"{method.upper()} {path}") + assert not offenders, f"still advertising FastAPI's default 422: {offenders}" + + +@pytest.mark.asyncio +async def test_events_stream_is_declared_as_sse(tmp_path, monkeypatch): + """It returns EventSourceResponse; advertising JSON makes consumers await + a body that never completes instead of opening a stream.""" + app = await _boot(tmp_path, monkeypatch) + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + spec = (await c.get("/docs/openapi.json")).json() + + content = (spec["paths"]["/v1/deployments/{deployment_id}/events"]["get"] + ["responses"]["200"]["content"]) + assert "text/event-stream" in content + assert "application/json" not in content