From dd0dd799a73b1e82b2151908de21d3aa17ac72ef Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Mon, 27 Jul 2026 16:27:33 -0700 Subject: [PATCH] =?UTF-8?q?feat(server):=20uniform=20error=20handling=20?= =?UTF-8?q?=E2=80=94=20domain=20errors=20to=20HTTP=20with=20stable=20codes?= =?UTF-8?q?=20(#31)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every failure the API can produce — a kernel domain error, a framework HTTPException, a request-validation failure, or an unhandled bug — is now rendered by one function into one ErrorBody {code, message, detail?}, declared in openapi.json's components and applied to every route. Clients branch on `code`, never on the status. DestructiveSchemaChange and SchemaChangeWouldOrphan are both 409, and only the first is retryable with a flag — a client branching on the status alone would loop against the second, which is the loop that error's own docstring warns about. Also introduces create_app(), keeping visionset.server.main:app importable for scripts/export_openapi.py, and gives #80's WorkspaceBusy its first caller. --- docs/README.md | 1 + docs/api.md | 172 +++++++++++++ openapi.json | 68 ++++++ src/visionset/server/__init__.py | 5 + src/visionset/server/errors.py | 404 +++++++++++++++++++++++++++++++ src/visionset/server/main.py | 47 +++- tests/server/test_errors.py | 379 +++++++++++++++++++++++++++++ 7 files changed, 1068 insertions(+), 8 deletions(-) create mode 100644 docs/api.md create mode 100644 src/visionset/server/errors.py create mode 100644 tests/server/test_errors.py diff --git a/docs/README.md b/docs/README.md index 96864973..0f5a6bd2 100644 --- a/docs/README.md +++ b/docs/README.md @@ -20,3 +20,4 @@ contracts (kernel purity, headless annotator) are described there and enforced i | [events.md](events.md) | Domain events: subscribing by type, why emission follows the commit, at-most-once delivery, and what an isolated subscriber failure does | | [persistence.md](persistence.md) | The metadata store: repositories, unit of work, table layout, migrations and `format_version` | | [examples.md](examples.md) | The two runnable examples: the whole cycle in one pass, ingest on its own, and what each is built to demonstrate | +| [api.md](api.md) | The REST surface: the one error body, why clients branch on `code` and not on the status, what decides 404 / 409 / 422, what a 5xx does and does not tell you, and which codes are worth retrying | diff --git a/docs/api.md b/docs/api.md new file mode 100644 index 00000000..65741b09 --- /dev/null +++ b/docs/api.md @@ -0,0 +1,172 @@ +# The REST API + +The REST surface is a thin client of the SDK: a route parses input, calls one service, and +shapes the output. This document covers the part of the contract that is *not* any single +endpoint — what a failure looks like, and how to read one. + +The routes themselves are described by [`openapi.json`](../openapi.json) at the repo root, which +is generated (`uv run python scripts/export_openapi.py`) and diffed in CI. Never hand-edit it. + +## The error body + +Every failure — a domain refusal, a missing route, a malformed payload, an unhandled bug — +arrives as one shape, declared in the spec as `ErrorBody`: + +```json +{ + "code": "BATCH_NOT_IN_ANNOTATION", + "message": "batch 3f2a… is approved, not in_annotation", + "detail": null +} +``` + +| Field | | +| --- | --- | +| `code` | Stable and machine-readable. **This is what a client branches on.** | +| `message` | A sentence for a human. The wording is not part of the contract and may change. | +| `detail` | Extra structure, or absent. Its shape depends on the `code` — it is not uniform. | + +### Branch on `code`, not on the status + +Statuses are coarse by design, and two errors sharing one is normal. The case that makes this +concrete: `DESTRUCTIVE_SCHEMA_CHANGE` and `SCHEMA_CHANGE_WOULD_ORPHAN` are **both 409**. The +first is retryable — resubmit with `allow_destructive=true` and it succeeds. The second has no +override at all, because annotations already depend on what the change would remove. A client +that saw 409 and retried with the flag would loop forever against the second one. The kernel's +own error hierarchy is built to prevent exactly that loop; over HTTP, the `code` is the only +thing carrying that distinction. + +## What decides the status + +Three rules, not one per error. + +**404 — the caller named something that is not there.** A project, batch, job, asset, source, +dataset, annotation or release that was never created, was deleted, or belongs to a different +workspace. Cross-scope references read as *missing*, never as *forbidden*: an asset in another +project is a 404, not a 403. `NO_SPLIT_RECIPE` is here too — a release published without a +recipe has no split sub-resource, and never will, because a release is immutable. + +**409 — the request is well-formed; the resource's state refuses it.** The remedy is to change +that state and resubmit the identical request: finish the outstanding jobs, approve the batch, +promote something into the dataset, pass `confirm=true`. Name and tag collisions are here, as is +`UNSERIALIZABLE_MANIFEST` — the request body is fine and the defect is in state stored long +before, so the remedy is to fix the annotation and publish again. + +**422 — the payload itself is wrong.** A blank name, a schema that declares two classes with one +name, an annotation that names a class the batch's pinned version does not have. Media failures +are here as well: `UNSUPPORTED_MEDIA` and `CORRUPT_MEDIA` describe a file that cannot become an +asset. They are deliberately *not* 415 — 415 is about the request's own `Content-Type`, and these +are raised while reading a file on disk that the operator pointed at. + +**503 — transient, and waiting helps.** Exactly one error: `WORKSPACE_BUSY`. See below. + +**500 — nothing the caller can do.** A corrupt or unreadable workspace, a store constraint no +service pre-checked, a missing ffmpeg, or a bug. + +### The two shapes of 422 + +Both are reachable on the same route, and they differ in `detail`: + +- **`VALIDATION_ERROR`** — the request failed pydantic validation before any service ran. + `detail.errors` carries pydantic's own list of per-field problems. +- **A domain refusal** (`INVALID_NAME`, `INVALID_SCHEMA`, `LABEL_CLASS_NOT_IN_SCHEMA`, …) — the + payload parsed, and a kernel rule rejected it. `detail` is usually `null`. + +Most malformed input arrives as the first: a `LabelClass` that cannot be constructed never +reaches a service to be refused by one. + +## The 5xx contract + +A 5xx body carries an **`incident_id`** in `detail`, and its `message` is a fixed generic +sentence. The real message and traceback go to the server log under the same id — so an operator +greps one string, and a response body never becomes a channel for filesystem paths, SQL text, or +a stack trace. + +Three errors opt out and expose their real message, each because that message *is* the remedy: + +| Code | Why the message is published | +| --- | --- | +| `WORKSPACE_BUSY` | Names the contention; and the whole point is that a retry works. | +| `WORKSPACE_FORMAT_TOO_NEW` | "Upgrade VisionSet to open it" is the entire fix. | +| `MEDIA_TOOL_UNAVAILABLE` | Carries the install hint. Without it the error says nothing an operator did not suspect. | + +A **mapped** 5xx keeps its own code (`WORKSPACE_CORRUPT`, `CONSTRAINT_VIOLATED`). An exception no +rule covers — a bug — gets `INTERNAL_ERROR`. That difference is how the two are told apart in a +log without reading the message. + +`MEDIA_TOOL_UNAVAILABLE` is a 500 rather than a 503 on purpose: 503 promises that waiting helps, +and no amount of retrying installs ffmpeg. + +## Retrying + +| Code | Status | How to retry | +| --- | --- | --- | +| `WORKSPACE_BUSY` | 503 | Wait. The response carries `Retry-After`, currently **5 seconds** — matched to the store's own busy timeout, because a client that gets this has *already* waited that long losing to another writer, and a shorter hint would aim a retry storm at the contention being reported. | +| `SCHEMA_VERSION_CONFLICT` | 409 | Immediately. Two writers computed the same next version and this one lost; a retry re-reads the maximum and lands on the one after. No `Retry-After`, because there is nothing to wait for. | +| `DESTRUCTIVE_SCHEMA_CHANGE` | 409 | With `allow_destructive=true`, if narrowing the contract is what you meant. | +| `CONFIRMATION_REQUIRED` | 409 | With `confirm=true`, after asking whoever is destroying the data. | + +Nothing else is retryable as-is. + +## The full table + +| Status | Codes | +| --- | --- | +| **404** | `PROJECT_NOT_FOUND` · `SCHEMA_NOT_FOUND` · `BATCH_NOT_FOUND` · `JOB_NOT_FOUND` · `INGEST_JOB_NOT_FOUND` · `ASSET_NOT_FOUND` · `SOURCE_NOT_FOUND` · `DATASET_NOT_FOUND` · `ANNOTATION_NOT_FOUND` · `RELEASE_NOT_FOUND` · `ASSET_NOT_IN_JOB` · `NO_SPLIT_RECIPE` · `NOT_FOUND` (no such route) | +| **405** | `METHOD_NOT_ALLOWED` | +| **401** | `UNAUTHORIZED` — with a `WWW-Authenticate: Bearer` challenge | +| **409** | `PROJECT_NAME_TAKEN` · `RELEASE_TAG_TAKEN` · `WORKSPACE_ALREADY_EXISTS` · `WORKSPACE_NOT_EMPTY` · `SCHEMA_VERSION_CONFLICT` · `INVALID_TRANSITION` · `BATCH_NOT_EDITABLE` · `BATCH_NOT_IN_ANNOTATION` · `BATCH_NOT_COMPLETE` · `JOB_NOT_COMPLETE` · `EMPTY_BATCH` · `EMPTY_RELEASE` · `CONFIRMATION_REQUIRED` · `DESTRUCTIVE_SCHEMA_CHANGE` · `SCHEMA_CHANGE_WOULD_ORPHAN` · `UNSERIALIZABLE_MANIFEST` | +| **422** | `VALIDATION_ERROR` · `INVALID_NAME` · `INVALID_SCHEMA` · `UNSUPPORTED_GEOMETRY` · `INVALID_ANNOTATION` · `LABEL_CLASS_NOT_IN_SCHEMA` · `DISALLOWED_GEOMETRY` · `MISSING_REQUIRED_ATTRIBUTE` · `UNKNOWN_ATTRIBUTE` · `INVALID_ATTRIBUTE_VALUE` · `INVALID_PARTITION` · `MEDIA_ERROR` · `UNSUPPORTED_MEDIA` · `CORRUPT_MEDIA` | +| **503** | `WORKSPACE_BUSY` | +| **500** | `WORKSPACE_CORRUPT` · `NOT_A_WORKSPACE` · `WORKSPACE_FORMAT_TOO_NEW` · `ENTITY_NOT_FOUND` · `ENTITY_ALREADY_EXISTS` · `CONSTRAINT_VIOLATED` · `MEDIA_TOOL_UNAVAILABLE` · `INTERNAL_ERROR` | + +`CORRUPT_MEDIA` and `UNSUPPORTED_MEDIA` carry `detail.reason`. The file's *name* is deliberately +absent from both the detail and the message: on the ingest path it is an absolute path inside a +directory the operator, not the client, pointed at. + +--- + +## For contributors + +### Adding an endpoint + +Raise the kernel's domain error and stop. The handlers registered by `create_app()` do the rest, +and a route that catches a domain error to translate it itself is how a second error shape gets +into the contract. + +`server/errors.py` holds one table, `ERROR_RULES`, with one entry per error class declared in +`kernel/errors.py`. `tests/server/test_errors.py` asserts that correspondence is **exact**, so a +new kernel error fails the suite until somebody maps it deliberately — which is the point. + +Codes are written out as literals rather than derived from the class name. A code is a public +contract keyed to a Python identifier, and deriving it means a pure refactor rename silently +breaks every client while passing every test. A test asserts each literal still matches its class +name today; when a class genuinely is renamed, add it to that test's `RENAMED` map and leave the +code alone. + +### Overriding a status for one route + +A couple of errors legitimately differ by route — an asset id in a path is a 404, and the same id +in a request body is a 422. Catch it and return the renderer directly: + +```python +try: + service.record(...) +except AssetNotInJob as exc: + return error_response(exc, status=422) +``` + +That is the whole escape hatch. Raising a bare `HTTPException` instead would produce a body in +the right shape but with a status-derived code nothing can branch on. + +### Two things that will bite + +**Do not commit a unit of work inside a `Depends(...)` teardown.** FastAPI gives yield-dependencies +their own exit stack, and an exception raised there after the response has started produces +`RuntimeError: Caught handled exception, but response already started` rather than an `ErrorBody` +— which is precisely when `WorkspaceBusy` fires. Yield the *service*; the kernel already commits +inside its own `unit_of_work()`. + +**422 is declared at app level, and that is load-bearing.** It displaces FastAPI's generated +`HTTPValidationError`, keeping that model — and the second error shape it implies — out of +`openapi.json` entirely. A test asserts it never comes back. diff --git a/openapi.json b/openapi.json index 4ec204ac..d9552cd6 100644 --- a/openapi.json +++ b/openapi.json @@ -1,4 +1,42 @@ { + "components": { + "schemas": { + "ErrorBody": { + "description": "The one error shape this API emits, at every status.", + "properties": { + "code": { + "description": "Stable machine-readable code. Branch on this, not on the status.", + "title": "Code", + "type": "string" + }, + "detail": { + "anyOf": [ + { + "additionalProperties": true, + "type": "object" + }, + { + "type": "null" + } + ], + "description": "Extra structure whose shape depends on the code; absent when there is none.", + "title": "Detail" + }, + "message": { + "description": "Human-readable sentence. Wording is not part of the contract.", + "title": "Message", + "type": "string" + } + }, + "required": [ + "code", + "message" + ], + "title": "ErrorBody", + "type": "object" + } + } + }, "info": { "description": "REST surface of the VisionSet SDK. The committed openapi.json is the contract.", "title": "Robomous VisionSet API", @@ -24,6 +62,36 @@ } }, "description": "Successful Response" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The request payload is not processable" + }, + "500": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "Unhandled server error, with an incident id" + }, + "503": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorBody" + } + } + }, + "description": "The workspace is busy; retry after the header says" } }, "summary": "Health" diff --git a/src/visionset/server/__init__.py b/src/visionset/server/__init__.py index 37c7e96f..5de04b7f 100644 --- a/src/visionset/server/__init__.py +++ b/src/visionset/server/__init__.py @@ -2,4 +2,9 @@ The OpenAPI spec exported from this app (repo-root ``openapi.json``) is a versioned public contract; the official UI has no private endpoints. + +Failures are part of that contract. A route raises a kernel domain error and +``errors.py`` turns it into an ``ErrorBody`` with a stable machine ``code``; +routes do not translate errors themselves and never raise ``HTTPException`` for +something the kernel already has a name for. See ``docs/api.md``. """ diff --git a/src/visionset/server/errors.py b/src/visionset/server/errors.py new file mode 100644 index 00000000..0029a0dd --- /dev/null +++ b/src/visionset/server/errors.py @@ -0,0 +1,404 @@ +# usage: from visionset.server.errors import ErrorBody, install_error_handlers +"""The API's error contract: one body, one table, one renderer. + +Every error a client can receive — a kernel domain error, a framework +``HTTPException``, a request-validation failure, or an unhandled bug — is +rendered by :func:`error_response` into one :class:`ErrorBody`. Nothing in this +package may invent a second error shape. + +**Clients branch on ``code``, never on the status.** Statuses are coarse by +design: ``DestructiveSchemaChange`` and ``SchemaChangeWouldOrphan`` are both +409, and the first is retryable with ``allow_destructive=True`` while the second +is not — a client that branched on 409 alone would retry the second one forever, +which is the loop that error's own docstring warns about. The per-class code is +the only thing that prevents it. + +Three rules place a domain error, not fifty: + +- **404** — the caller named something that is not there. +- **409** — the request is well-formed; the *resource's state* refuses it, and + the remedy is to change that state and resubmit the identical request. +- **422** — the payload itself is wrong. + +5xx is opaque by default: the body carries a generic sentence and an +``incident_id``, and the real message and traceback go to the log. Three errors +opt out, each because its message *is* the operator's remedy — see +``expose_message`` below. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from http import HTTPStatus +from typing import Any, Final +from uuid import uuid4 + +from fastapi import FastAPI +from fastapi.encoders import jsonable_encoder +from fastapi.exceptions import RequestValidationError +from fastapi.utils import is_body_allowed_for_status_code +from pydantic import BaseModel, Field +from starlette.exceptions import HTTPException +from starlette.requests import Request +from starlette.responses import JSONResponse, Response + +from visionset.kernel import ( + AnnotationNotFound, + AssetNotFound, + AssetNotInJob, + BatchNotComplete, + BatchNotEditable, + BatchNotFound, + BatchNotInAnnotation, + ConfirmationRequired, + ConstraintViolated, + CorruptMedia, + DatasetNotFound, + DestructiveSchemaChange, + DisallowedGeometry, + EmptyBatch, + EmptyRelease, + EntityAlreadyExists, + EntityNotFound, + IngestJobNotFound, + InvalidAnnotation, + InvalidAttributeValue, + InvalidName, + InvalidPartition, + InvalidSchema, + InvalidTransition, + JobNotComplete, + JobNotFound, + LabelClassNotInSchema, + MediaError, + MediaToolUnavailable, + MissingRequiredAttribute, + NoSplitRecipe, + NotAWorkspace, + ProjectNameTaken, + ProjectNotFound, + ReleaseNotFound, + ReleaseTagTaken, + SchemaChangeWouldOrphan, + SchemaNotFound, + SchemaVersionConflict, + SourceNotFound, + UnknownAttribute, + UnserializableManifest, + UnsupportedGeometry, + UnsupportedMedia, + VisionSetError, + WorkspaceAlreadyExists, + WorkspaceBusy, + WorkspaceCorrupt, + WorkspaceFormatTooNew, + WorkspaceNotEmpty, +) + +_logger = logging.getLogger(__name__) +"""Never call ``logging.basicConfig`` here — records propagate to root, which +uvicorn configures. The kernel's event-bus logger follows the same rule.""" + +RETRY_AFTER_SECONDS: Final = 5 +"""How long a client is told to wait after a 503. + +Matched to the store's ``DEFAULT_BUSY_TIMEOUT_MS`` (5 s), *not* imported from +it: the server is not bound to one adapter. A ``WorkspaceBusy`` client has +already waited out that timeout losing to another writer, so a shorter hint +would aim a retry storm at the exact contention being reported. +""" + +UNMAPPED_CODE: Final = "INTERNAL_ERROR" +"""The code for an exception no rule covers — a bug, by definition. + +A *mapped* 5xx keeps its own code (``WORKSPACE_CORRUPT``), so the two are told +apart in a log without reading the message. +""" + +OPAQUE_MESSAGE: Final = "The server failed to handle the request." + + +# Named ErrorBody rather than Error because it becomes a public identifier in +# the generated TypeScript client, where Error is taken. The docstring is short +# and plain on purpose: it is copied verbatim into openapi.json, so RST markup +# would ship as literal backticks to every consumer of the contract. +class ErrorBody(BaseModel): + """The one error shape this API emits, at every status.""" + + code: str = Field( + description="Stable machine-readable code. Branch on this, not on the status." + ) + message: str = Field( + description="Human-readable sentence. Wording is not part of the contract." + ) + detail: dict[str, Any] | None = Field( + default=None, + description="Extra structure whose shape depends on the code; absent when there is none.", + ) + + +@dataclass(frozen=True, slots=True) +class ErrorRule: + """What one domain error becomes over HTTP.""" + + status: int + code: str + retry_after: int | None = None + """Emit a ``Retry-After`` header. Only for an error a *wait* actually helps.""" + expose_message: bool = False + """5xx only: let ``str(exc)`` reach the client instead of the opaque sentence.""" + + +# The table is complete: one entry per concrete subclass declared in +# ``kernel/errors.py``. ``VisionSetError`` itself is deliberately absent, and +# ``tests/server/test_errors.py`` asserts exact equality against that module — +# so a new kernel error fails the suite until somebody maps it on purpose. +# +# Codes are written out rather than derived from the class name. Derivation +# cannot drift, but a code is a public contract keyed to a Python identifier: a +# pure refactor rename would silently break every client and pass every test. +# A test asserts each literal equals the SCREAMING_SNAKE of its class today, so +# the drift protection survives without the fragility. +ERROR_RULES: Final[dict[type[VisionSetError], ErrorRule]] = { + # --- 404: the caller named something that is not there ---------------- + ProjectNotFound: ErrorRule(404, "PROJECT_NOT_FOUND"), + SchemaNotFound: ErrorRule(404, "SCHEMA_NOT_FOUND"), + BatchNotFound: ErrorRule(404, "BATCH_NOT_FOUND"), + JobNotFound: ErrorRule(404, "JOB_NOT_FOUND"), + IngestJobNotFound: ErrorRule(404, "INGEST_JOB_NOT_FOUND"), + AssetNotFound: ErrorRule(404, "ASSET_NOT_FOUND"), + SourceNotFound: ErrorRule(404, "SOURCE_NOT_FOUND"), + DatasetNotFound: ErrorRule(404, "DATASET_NOT_FOUND"), + AnnotationNotFound: ErrorRule(404, "ANNOTATION_NOT_FOUND"), + ReleaseNotFound: ErrorRule(404, "RELEASE_NOT_FOUND"), + # A job's assets are fixed at approval, so an asset outside the segment is + # a sub-resource that does not exist — the "reads as missing, not as + # forbidden" rule one scope down. A route that takes the asset id in a + # *body* rather than a path should override to 422 via ``error_response``. + AssetNotInJob: ErrorRule(404, "ASSET_NOT_IN_JOB"), + # Not a 409: a release is immutable, so its state will never change and + # "resolve the conflict and resubmit" is a promise that cannot be kept. The + # docstring's remedy is a *different* release. The code is what tells this + # apart from RELEASE_NOT_FOUND, which is the case codes exist for. + NoSplitRecipe: ErrorRule(404, "NO_SPLIT_RECIPE"), + # --- 409: well-formed request, the resource's state refuses it --------- + ProjectNameTaken: ErrorRule(409, "PROJECT_NAME_TAKEN"), + ReleaseTagTaken: ErrorRule(409, "RELEASE_TAG_TAKEN"), + WorkspaceAlreadyExists: ErrorRule(409, "WORKSPACE_ALREADY_EXISTS"), + WorkspaceNotEmpty: ErrorRule(409, "WORKSPACE_NOT_EMPTY"), + # Retryable, but immediately rather than after a wait — a re-read lands on + # N + 2 — so no Retry-After. Which codes are retryable is documented in + # docs/api.md; a `retryable` field on the public body would widen it for one case. + SchemaVersionConflict: ErrorRule(409, "SCHEMA_VERSION_CONFLICT"), + InvalidTransition: ErrorRule(409, "INVALID_TRANSITION"), + BatchNotEditable: ErrorRule(409, "BATCH_NOT_EDITABLE"), + BatchNotInAnnotation: ErrorRule(409, "BATCH_NOT_IN_ANNOTATION"), + BatchNotComplete: ErrorRule(409, "BATCH_NOT_COMPLETE"), + JobNotComplete: ErrorRule(409, "JOB_NOT_COMPLETE"), + EmptyBatch: ErrorRule(409, "EMPTY_BATCH"), + EmptyRelease: ErrorRule(409, "EMPTY_RELEASE"), + ConfirmationRequired: ErrorRule(409, "CONFIRMATION_REQUIRED"), + DestructiveSchemaChange: ErrorRule(409, "DESTRUCTIVE_SCHEMA_CHANGE"), + SchemaChangeWouldOrphan: ErrorRule(409, "SCHEMA_CHANGE_WOULD_ORPHAN"), + # Not a 422: the request body is valid, and the defect is in state that was + # written and stored long before — a NaN coordinate only surfaces when a + # release tries to freeze it. The remedy is "fix the annotation and publish + # again", which is change-the-state-and-resubmit. + UnserializableManifest: ErrorRule(409, "UNSERIALIZABLE_MANIFEST"), + # --- 422: the payload itself is wrong ---------------------------------- + InvalidName: ErrorRule(422, "INVALID_NAME"), + InvalidSchema: ErrorRule(422, "INVALID_SCHEMA"), + UnsupportedGeometry: ErrorRule(422, "UNSUPPORTED_GEOMETRY"), + InvalidAnnotation: ErrorRule(422, "INVALID_ANNOTATION"), + LabelClassNotInSchema: ErrorRule(422, "LABEL_CLASS_NOT_IN_SCHEMA"), + DisallowedGeometry: ErrorRule(422, "DISALLOWED_GEOMETRY"), + MissingRequiredAttribute: ErrorRule(422, "MISSING_REQUIRED_ATTRIBUTE"), + UnknownAttribute: ErrorRule(422, "UNKNOWN_ATTRIBUTE"), + InvalidAttributeValue: ErrorRule(422, "INVALID_ATTRIBUTE_VALUE"), + InvalidPartition: ErrorRule(422, "INVALID_PARTITION"), + MediaError: ErrorRule(422, "MEDIA_ERROR"), + # Not a 415: every raise site reads a file *on disk* during ingest, and 415 + # is about the request's own Content-Type. On a future direct-upload route + # 415 becomes right for one of this error's three readings and 413 for + # another — which is what ``error_response(exc, status=...)`` is for. + UnsupportedMedia: ErrorRule(422, "UNSUPPORTED_MEDIA"), + CorruptMedia: ErrorRule(422, "CORRUPT_MEDIA"), + # --- 503: transient, and a wait genuinely helps ------------------------ + WorkspaceBusy: ErrorRule( + 503, "WORKSPACE_BUSY", retry_after=RETRY_AFTER_SECONDS, expose_message=True + ), + # --- 5xx: nothing the caller can fix ----------------------------------- + WorkspaceCorrupt: ErrorRule(500, "WORKSPACE_CORRUPT"), + # Deployment conditions, not client errors, and neither is transient — the + # only licence for a 503 in the kernel is WorkspaceBusy's "transient, unlike + # WorkspaceCorrupt … where corruption gets a hard failure". + NotAWorkspace: ErrorRule(500, "NOT_A_WORKSPACE"), # messages embed the server's own path + WorkspaceFormatTooNew: ErrorRule(500, "WORKSPACE_FORMAT_TOO_NEW", expose_message=True), + # A row missing where the store required one, or a primary-key collision on + # a kernel-generated UUID: a programming error, per ProjectNotFound's + # docstring ("a delivery surface turns it into a 404, not a 500" — said of + # the *other* one). + EntityNotFound: ErrorRule(500, "ENTITY_NOT_FOUND"), + EntityAlreadyExists: ErrorRule(500, "ENTITY_ALREADY_EXISTS"), + # Services pre-check rather than relying on the write to fail, and the two + # that translate a constraint each own exactly one index — so anything + # reaching here is a guard nobody wrote. Opaque as well as 500: the message + # is ``str(exc.orig)``, raw SQLite text naming our own tables and columns. + ConstraintViolated: ErrorRule(500, "CONSTRAINT_VIOLATED"), + # Not a 503, despite being about availability: 503 promises transience, and + # retrying never succeeds until an operator installs the binary. The message + # is exposed because it carries the install hint, which its docstring calls + # the whole reason the message exists. + MediaToolUnavailable: ErrorRule(500, "MEDIA_TOOL_UNAVAILABLE", expose_message=True), +} + +ERROR_RESPONSES: Final[dict[int | str, dict[str, Any]]] = { + 401: {"model": ErrorBody, "description": "Missing or invalid bearer token"}, + 404: {"model": ErrorBody, "description": "No such resource"}, + 409: {"model": ErrorBody, "description": "The resource's state refuses this request"}, + 422: {"model": ErrorBody, "description": "The request payload is not processable"}, + 500: {"model": ErrorBody, "description": "Unhandled server error, with an incident id"}, + 503: {"model": ErrorBody, "description": "The workspace is busy; retry after the header says"}, +} +"""Documented responses, keyed by status. + +``create_app`` passes the *universal* subset at app level — see +``UNIVERSAL_ERROR_RESPONSES``. A route spreads the rest of these into its own +``responses=`` for the statuses it can actually produce: a public route cannot +401, and only a route addressing an id can 404. +""" + +UNIVERSAL_ERROR_RESPONSES: Final[dict[int | str, dict[str, Any]]] = { + status: ERROR_RESPONSES[status] for status in (422, 500, 503) +} +"""What any route can emit, applied at app level. + +Declaring **422** here is load-bearing beyond documentation: it overrides +FastAPI's generated ``HTTPValidationError`` response, which keeps that model — +and the second error shape it implies — out of ``openapi.json`` entirely. This +is what makes "one error body, used everywhere" true by construction rather than +by every future route remembering. +""" + + +def rule_for(exc: BaseException) -> ErrorRule | None: + """The rule for ``exc``, or ``None`` if nothing in the table covers it. + + Walks the MRO, so a subclass declared outside ``kernel/errors.py`` inherits + its nearest mapped ancestor's answer — which is the same promise + ``InvalidAnnotation`` and ``MediaError`` make in their own docstrings, that a + surface can treat the whole family at once. + """ + for cls in type(exc).__mro__: + rule = ERROR_RULES.get(cls) + if rule is not None: + return rule + return None + + +def _detail_for(exc: BaseException) -> dict[str, Any] | None: + if isinstance(exc, MediaError): + # ``reason`` only. ``name`` is "a path for a file on disk" from a + # directory the *operator* pointed at, not the client — putting it in a + # response body hands out server filesystem layout. Do not add it back. + return {"reason": exc.reason} + return None + + +def _message_for(exc: BaseException) -> str: + if isinstance(exc, MediaError): + # NOT ``str(exc)``, which is ``f"{name}: {reason}"`` — dropping ``name`` + # from ``detail`` while leaving it in the message would hide nothing. + # The kernel already separated the two ("reason never repeats the + # name"); this is the surface taking the half that is safe to publish. + return exc.reason + return str(exc) + + +def error_response(exc: BaseException, *, status: int | None = None) -> JSONResponse: + """Render ``exc`` as an :class:`ErrorBody` response. + + ``status`` overrides the table for one call site. That escape hatch exists + because a couple of domain errors legitimately differ by route — an asset id + in a path is a 404 where the same id in a request body is a 422 — and the + alternative is stray ``HTTPException``s that speak a different shape. + """ + rule = rule_for(exc) + code = rule.code if rule is not None else UNMAPPED_CODE + resolved = status if status is not None else (rule.status if rule is not None else 500) + + detail = _detail_for(exc) + if resolved >= 500: + incident_id = uuid4().hex + _logger.exception( + "%s failed the request (incident %s)", type(exc).__name__, incident_id, exc_info=exc + ) + message = _message_for(exc) if rule is not None and rule.expose_message else OPAQUE_MESSAGE + detail = {**(detail or {}), "incident_id": incident_id} + else: + message = _message_for(exc) + + headers: dict[str, str] = {} + if rule is not None and rule.retry_after is not None: + headers["Retry-After"] = str(rule.retry_after) + + body = ErrorBody(code=code, message=message, detail=detail) + return JSONResponse(body.model_dump(mode="json"), status_code=resolved, headers=headers) + + +_KNOWN_STATUSES: Final = frozenset(status.value for status in HTTPStatus) + + +# Handlers take ``Exception`` and narrow inside: Starlette's ``ExceptionHandler`` +# is typed that way, and a narrower annotation is contravariance-incompatible — +# mypy rejects it at ``add_exception_handler``. Narrowing beats a ``type: ignore``. + + +async def _domain_error_handler(request: Request, exc: Exception) -> Response: + return error_response(exc) + + +async def _http_exception_handler(request: Request, exc: Exception) -> Response: + assert isinstance(exc, HTTPException) + headers = exc.headers # keeps WWW-Authenticate on a 401 + if not is_body_allowed_for_status_code(exc.status_code): + return Response(status_code=exc.status_code, headers=headers) + code = HTTPStatus(exc.status_code).name if exc.status_code in _KNOWN_STATUSES else "HTTP_ERROR" + body = ErrorBody(code=code, message=str(exc.detail)) + return JSONResponse(body.model_dump(mode="json"), status_code=exc.status_code, headers=headers) + + +async def _validation_error_handler(request: Request, exc: Exception) -> Response: + assert isinstance(exc, RequestValidationError) + # jsonable_encoder is not cosmetic: pydantic's ``ctx`` can hold objects json + # cannot serialize. FastAPI's own handler does exactly this. + body = ErrorBody( + code="VALIDATION_ERROR", + message="The request payload is not processable.", + detail={"errors": jsonable_encoder(exc.errors())}, + ) + return JSONResponse(body.model_dump(mode="json"), status_code=422) + + +async def _unhandled_error_handler(request: Request, exc: Exception) -> Response: + return error_response(exc, status=500) + + +def install_error_handlers(app: FastAPI) -> None: + """Register the four handlers that make every error one shape. + + Also usable on a throwaway probe app, which is how this module is tested + without adding routes to the real one and moving ``openapi.json``. + """ + app.add_exception_handler(VisionSetError, _domain_error_handler) + # Starlette's own class, NOT fastapi's. The router raises the Starlette one + # for an unknown path and for a 405, and fastapi's is a *subclass* — keying + # on the subclass would leave those two answering with FastAPI's default + # ``{"detail": ...}`` while everything else answered with ErrorBody. + app.add_exception_handler(HTTPException, _http_exception_handler) + app.add_exception_handler(RequestValidationError, _validation_error_handler) + # This one lands in ServerErrorMiddleware, *outside* the user middleware + # stack, so middleware added later (CORS, say) will not run on it. That is + # why the mapped-5xx path above stays separate rather than being folded in. + app.add_exception_handler(Exception, _unhandled_error_handler) diff --git a/src/visionset/server/main.py b/src/visionset/server/main.py index 1abac3bc..2fadb868 100644 --- a/src/visionset/server/main.py +++ b/src/visionset/server/main.py @@ -5,11 +5,12 @@ import os from typing import Annotated -from fastapi import Depends, FastAPI, HTTPException, status +from fastapi import APIRouter, Depends, FastAPI, HTTPException, status from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer from visionset import __version__ from visionset.kernel.ports import AuthProvider +from visionset.server.errors import UNIVERSAL_ERROR_RESPONSES, install_error_handlers class EnvTokenAuthProvider: @@ -30,7 +31,12 @@ def verify(self, token: str) -> bool: async def require_token( credentials: Annotated[HTTPAuthorizationCredentials | None, Depends(_bearer)], ) -> str: - """Bearer-token dependency for every future non-public endpoint.""" + """Bearer-token dependency for every future non-public endpoint. + + The ``HTTPException`` is rendered as an ``ErrorBody`` by the handler + :func:`create_app` installs, headers included — which is what keeps the + ``WWW-Authenticate`` challenge on the response. + """ if credentials is None or not _auth_provider.verify(credentials.credentials): raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, @@ -40,14 +46,39 @@ async def require_token( return credentials.credentials -app = FastAPI( - title="Robomous VisionSet API", - version=__version__, - description="REST surface of the VisionSet SDK. The committed openapi.json is the contract.", -) +DESCRIPTION = "REST surface of the VisionSet SDK. The committed openapi.json is the contract." +router = APIRouter() -@app.get("/health") + +@router.get("/health") async def health() -> dict[str, str]: """Liveness probe. Public — no token required.""" return {"status": "ok", "version": __version__} + + +def create_app() -> FastAPI: + """Build the application. + + A factory rather than a bare module-level literal, so a test or a future + entry point can build an app with its own wiring. The module-level ``app`` + below stays regardless: ``scripts/export_openapi.py`` imports it by name, + and so does ``uvicorn visionset.server.main:app``. + + ``responses=`` is applied here rather than route by route on purpose. It + puts ``ErrorBody`` in ``components.schemas`` and displaces FastAPI's + generated ``HTTPValidationError``, so no route can quietly document a second + error shape. + """ + app = FastAPI( + title="Robomous VisionSet API", + version=__version__, + description=DESCRIPTION, + responses=UNIVERSAL_ERROR_RESPONSES, + ) + install_error_handlers(app) + app.include_router(router) + return app + + +app = create_app() diff --git a/tests/server/test_errors.py b/tests/server/test_errors.py new file mode 100644 index 00000000..9122c202 --- /dev/null +++ b/tests/server/test_errors.py @@ -0,0 +1,379 @@ +"""The API error contract: the table is exhaustive, and the handlers obey it. + +Behaviour is exercised on throwaway probe apps, the pattern ``test_health.py`` +established: mounting routes that raise on the real ``app`` would put them in +``openapi.json`` and trip the CI drift gate. +""" + +from __future__ import annotations + +import inspect +import re +from collections.abc import Iterator +from typing import Annotated + +import pytest +from fastapi import Depends, FastAPI +from fastapi.testclient import TestClient +from pydantic import BaseModel + +from visionset.kernel import ( + AssetNotInJob, + CorruptMedia, + MediaToolUnavailable, + ProjectNotFound, + VisionSetError, + WorkspaceBusy, + WorkspaceCorrupt, +) +from visionset.kernel import errors as kernel_errors +from visionset.server.errors import ( + ERROR_RULES, + OPAQUE_MESSAGE, + RETRY_AFTER_SECONDS, + UNMAPPED_CODE, + ErrorBody, + ErrorRule, + error_response, + install_error_handlers, + rule_for, +) +from visionset.server.main import app, require_token + +# --- the table ------------------------------------------------------------ + +# Class name -> (status, code). Written out rather than computed, so a status +# change is a readable diff in a review instead of a silent one. +EXPECTED: dict[str, tuple[int, str]] = { + # 404 — the caller named something that is not there + "ProjectNotFound": (404, "PROJECT_NOT_FOUND"), + "SchemaNotFound": (404, "SCHEMA_NOT_FOUND"), + "BatchNotFound": (404, "BATCH_NOT_FOUND"), + "JobNotFound": (404, "JOB_NOT_FOUND"), + "IngestJobNotFound": (404, "INGEST_JOB_NOT_FOUND"), + "AssetNotFound": (404, "ASSET_NOT_FOUND"), + "SourceNotFound": (404, "SOURCE_NOT_FOUND"), + "DatasetNotFound": (404, "DATASET_NOT_FOUND"), + "AnnotationNotFound": (404, "ANNOTATION_NOT_FOUND"), + "ReleaseNotFound": (404, "RELEASE_NOT_FOUND"), + "AssetNotInJob": (404, "ASSET_NOT_IN_JOB"), + "NoSplitRecipe": (404, "NO_SPLIT_RECIPE"), + # 409 — well-formed request, the resource's state refuses it + "ProjectNameTaken": (409, "PROJECT_NAME_TAKEN"), + "ReleaseTagTaken": (409, "RELEASE_TAG_TAKEN"), + "WorkspaceAlreadyExists": (409, "WORKSPACE_ALREADY_EXISTS"), + "WorkspaceNotEmpty": (409, "WORKSPACE_NOT_EMPTY"), + "SchemaVersionConflict": (409, "SCHEMA_VERSION_CONFLICT"), + "InvalidTransition": (409, "INVALID_TRANSITION"), + "BatchNotEditable": (409, "BATCH_NOT_EDITABLE"), + "BatchNotInAnnotation": (409, "BATCH_NOT_IN_ANNOTATION"), + "BatchNotComplete": (409, "BATCH_NOT_COMPLETE"), + "JobNotComplete": (409, "JOB_NOT_COMPLETE"), + "EmptyBatch": (409, "EMPTY_BATCH"), + "EmptyRelease": (409, "EMPTY_RELEASE"), + "ConfirmationRequired": (409, "CONFIRMATION_REQUIRED"), + "DestructiveSchemaChange": (409, "DESTRUCTIVE_SCHEMA_CHANGE"), + "SchemaChangeWouldOrphan": (409, "SCHEMA_CHANGE_WOULD_ORPHAN"), + "UnserializableManifest": (409, "UNSERIALIZABLE_MANIFEST"), + # 422 — the payload itself is wrong + "InvalidName": (422, "INVALID_NAME"), + "InvalidSchema": (422, "INVALID_SCHEMA"), + "UnsupportedGeometry": (422, "UNSUPPORTED_GEOMETRY"), + "InvalidAnnotation": (422, "INVALID_ANNOTATION"), + "LabelClassNotInSchema": (422, "LABEL_CLASS_NOT_IN_SCHEMA"), + "DisallowedGeometry": (422, "DISALLOWED_GEOMETRY"), + "MissingRequiredAttribute": (422, "MISSING_REQUIRED_ATTRIBUTE"), + "UnknownAttribute": (422, "UNKNOWN_ATTRIBUTE"), + "InvalidAttributeValue": (422, "INVALID_ATTRIBUTE_VALUE"), + "InvalidPartition": (422, "INVALID_PARTITION"), + "MediaError": (422, "MEDIA_ERROR"), + "UnsupportedMedia": (422, "UNSUPPORTED_MEDIA"), + "CorruptMedia": (422, "CORRUPT_MEDIA"), + # 503 — transient, and a wait genuinely helps + "WorkspaceBusy": (503, "WORKSPACE_BUSY"), + # 5xx — nothing the caller can fix + "WorkspaceCorrupt": (500, "WORKSPACE_CORRUPT"), + "NotAWorkspace": (500, "NOT_A_WORKSPACE"), + "WorkspaceFormatTooNew": (500, "WORKSPACE_FORMAT_TOO_NEW"), + "EntityNotFound": (500, "ENTITY_NOT_FOUND"), + "EntityAlreadyExists": (500, "ENTITY_ALREADY_EXISTS"), + "ConstraintViolated": (500, "CONSTRAINT_VIOLATED"), + "MediaToolUnavailable": (500, "MEDIA_TOOL_UNAVAILABLE"), +} + +# A code outlives the class name it was derived from. Rename a class and its +# code stays put: add the class here, do not change the string clients read. +RENAMED: dict[str, str] = {} + + +def declared_errors() -> dict[str, type[VisionSetError]]: + """Every error class declared in ``kernel/errors.py``, base included. + + Read off the module rather than ``VisionSetError.__subclasses__()``: that + only sees classes something has imported, and it also sees subclasses + defined in other test modules, so the answer would depend on collection + order. + """ + return { + name: obj + for name, obj in vars(kernel_errors).items() + if inspect.isclass(obj) and issubclass(obj, VisionSetError) + } + + +def screaming_snake(name: str) -> str: + return re.sub(r"(? None: + declared = set(declared_errors()) - {"VisionSetError"} + assert {cls.__name__ for cls in ERROR_RULES} == declared + # The base stays out so an unmapped error cannot inherit an answer and pass + # this test by accident. + assert VisionSetError not in ERROR_RULES + + +def test_the_status_and_code_of_every_error() -> None: + resolved = {name: (rule.status, rule.code) for name, rule in _rules_by_name().items()} + assert resolved == EXPECTED + + +def test_codes_are_unique() -> None: + codes = [rule.code for rule in ERROR_RULES.values()] + assert len(set(codes)) == len(codes) + + +def test_codes_still_match_their_class_names() -> None: + drifted = { + name: rule.code + for name, rule in _rules_by_name().items() + if name not in RENAMED and rule.code != screaming_snake(name) + } + assert drifted == {} + + +def test_every_mapped_error_can_be_constructed_with_one_argument() -> None: + # ``MediaError`` is the only kernel error with a constructor; a second one + # would break every caller that raises by message alone, this walk included. + for cls in ERROR_RULES: + assert isinstance(cls("boom"), VisionSetError) + + +def test_only_transient_errors_carry_a_retry_after() -> None: + assert {cls.__name__ for cls, rule in ERROR_RULES.items() if rule.retry_after} == { + "WorkspaceBusy" + } + + +def test_message_exposure_is_opt_in_and_only_for_5xx() -> None: + exposed = {cls.__name__ for cls, rule in ERROR_RULES.items() if rule.expose_message} + assert exposed == {"WorkspaceBusy", "WorkspaceFormatTooNew", "MediaToolUnavailable"} + assert all(rule.status >= 500 for rule in ERROR_RULES.values() if rule.expose_message) + + +def test_rule_for_walks_the_mro() -> None: + class Custom(ProjectNotFound): + """A subclass declared outside kernel/errors.py inherits the family's answer.""" + + rule = rule_for(Custom("x")) + assert rule is not None + assert (rule.status, rule.code) == (404, "PROJECT_NOT_FOUND") + + +def test_rule_for_declines_an_exception_it_does_not_know() -> None: + assert rule_for(RuntimeError("x")) is None + + +def _rules_by_name() -> dict[str, ErrorRule]: + return {cls.__name__: rule for cls, rule in ERROR_RULES.items()} + + +# --- the handlers --------------------------------------------------------- + + +class Payload(BaseModel): + count: int + + +@pytest.fixture() +def probe() -> Iterator[TestClient]: + """A throwaway app carrying one route per error path under test.""" + probe_app = FastAPI() + install_error_handlers(probe_app) + + @probe_app.get("/missing") + async def missing() -> None: + raise ProjectNotFound("no project 'x'") + + @probe_app.get("/busy") + async def busy() -> None: + raise WorkspaceBusy("the workspace is held by another writer") + + @probe_app.get("/corrupt") + async def corrupt() -> None: + raise WorkspaceCorrupt("/srv/data/visionset.db is not a database") + + @probe_app.get("/no-ffmpeg") + async def no_ffmpeg() -> None: + raise MediaToolUnavailable("ffmpeg is not installed; brew install ffmpeg") + + @probe_app.get("/bad-media") + async def bad_media() -> None: + raise CorruptMedia("truncated at byte 12", name="/srv/incoming/clip.mp4") + + @probe_app.get("/boom") + async def boom() -> None: + raise RuntimeError("a bug with a revealing message") + + @probe_app.get("/override") + async def override() -> None: + # The escape hatch: the same error is a 404 addressed as a sub-resource + # and a 422 when the id arrived in a request body. + return error_response(AssetNotInJob("asset 7 is not in this job"), status=422) + + @probe_app.post("/typed") + async def typed(payload: Payload) -> dict[str, int]: + return {"count": payload.count} + + # ``raise_server_exceptions=False`` is required: ServerErrorMiddleware always + # re-raises after running the Exception handler, so the default client would + # see the exception instead of the response. + with TestClient(probe_app, raise_server_exceptions=False) as client: + yield client + + +def test_a_domain_error_becomes_its_mapped_status_and_code(probe: TestClient) -> None: + response = probe.get("/missing") + assert response.status_code == 404 + assert response.json() == { + "code": "PROJECT_NOT_FOUND", + "message": "no project 'x'", + "detail": None, + } + + +def test_a_4xx_body_validates_as_the_declared_schema(probe: TestClient) -> None: + assert ErrorBody.model_validate(probe.get("/missing").json()).code == "PROJECT_NOT_FOUND" + + +def test_workspace_busy_is_a_503_that_says_when_to_come_back(probe: TestClient) -> None: + response = probe.get("/busy") + assert response.status_code == 503 + assert response.headers["Retry-After"] == str(RETRY_AFTER_SECONDS) + # Transient and actionable, so the message is exposed rather than swallowed. + assert response.json()["message"] == "the workspace is held by another writer" + + +def test_a_mapped_500_is_opaque_but_keeps_its_code( + probe: TestClient, caplog: pytest.LogCaptureFixture +) -> None: + with caplog.at_level("ERROR"): + response = probe.get("/corrupt") + assert response.status_code == 500 + body = response.json() + assert body["code"] == "WORKSPACE_CORRUPT" + assert body["message"] == OPAQUE_MESSAGE + assert "visionset.db" not in response.text # the server's own path stays server-side + incident_id = body["detail"]["incident_id"] + # The message the client did not get is the one an operator greps for. + assert incident_id in caplog.text + assert "is not a database" in caplog.text + + +def test_an_error_whose_message_is_the_remedy_opts_out_of_opacity(probe: TestClient) -> None: + response = probe.get("/no-ffmpeg") + assert response.status_code == 500 + assert "brew install ffmpeg" in response.json()["message"] + assert response.json()["detail"]["incident_id"] # still incident-tracked + + +def test_a_media_error_reports_its_reason_and_never_its_path(probe: TestClient) -> None: + response = probe.get("/bad-media") + assert response.status_code == 422 + assert response.json()["detail"] == {"reason": "truncated at byte 12"} + # ``str(exc)`` is ": ", so the message has to be the reason + # alone — dropping the name from ``detail`` and leaving it here would hide + # nothing at all. + assert response.json()["message"] == "truncated at byte 12" + assert "/srv/incoming" not in response.text + + +def test_an_unmapped_exception_is_a_500_that_reveals_nothing(probe: TestClient) -> None: + response = probe.get("/boom") + assert response.status_code == 500 + body = response.json() + assert body["code"] == UNMAPPED_CODE + assert body["message"] == OPAQUE_MESSAGE + assert "revealing" not in response.text + assert "Traceback" not in response.text + + +def test_a_route_may_override_the_table_without_losing_the_code(probe: TestClient) -> None: + response = probe.get("/override") + assert response.status_code == 422 + assert response.json()["code"] == "ASSET_NOT_IN_JOB" + + +def test_a_validation_failure_speaks_the_same_schema(probe: TestClient) -> None: + response = probe.post("/typed", json={"count": "not a number"}) + assert response.status_code == 422 + body = ErrorBody.model_validate(response.json()) + assert body.code == "VALIDATION_ERROR" + assert body.detail is not None + assert body.detail["errors"] + + +def test_an_unknown_path_speaks_the_same_schema(probe: TestClient) -> None: + # Starlette's router raises its *own* HTTPException here; a handler keyed on + # fastapi's subclass would leave this answering {"detail": "Not Found"}. + response = probe.get("/nothing-here") + assert response.status_code == 404 + assert response.json() == {"code": "NOT_FOUND", "message": "Not Found", "detail": None} + + +def test_a_wrong_method_speaks_the_same_schema(probe: TestClient) -> None: + response = probe.post("/missing") + assert response.status_code == 405 + assert response.json()["code"] == "METHOD_NOT_ALLOWED" + + +# --- the 401, which #25 builds on ---------------------------------------- + + +def test_a_401_carries_the_error_body_and_keeps_its_challenge() -> None: + probe_app = FastAPI() + install_error_handlers(probe_app) + + @probe_app.get("/protected") + async def protected(token: Annotated[str, Depends(require_token)]) -> dict[str, bool]: + return {"ok": True} + + response = TestClient(probe_app).get("/protected") + assert response.status_code == 401 + assert response.json() == { + "code": "UNAUTHORIZED", + "message": "Invalid or missing bearer token", + "detail": None, + } + # The challenge survives the reshaping — that is the whole reason the + # handler forwards ``exc.headers``. + assert response.headers["WWW-Authenticate"] == "Bearer" + + +# --- the contract in openapi.json ---------------------------------------- + + +def test_the_error_body_is_the_only_error_schema_in_the_contract() -> None: + schemas = app.openapi()["components"]["schemas"] + assert "ErrorBody" in schemas + # Declaring 422 at app level displaces FastAPI's generated model. If this + # ever comes back, some route is documenting a second error shape. + assert "HTTPValidationError" not in schemas + assert "ValidationError" not in schemas + + +def test_every_route_documents_the_universal_error_responses() -> None: + for path, operations in app.openapi()["paths"].items(): + for method, operation in operations.items(): + declared = set(operation["responses"]) + assert {"422", "500", "503"} <= declared, f"{method.upper()} {path}"