diff --git a/docs/agents.md b/docs/agents.md index a82ca16..2c233c7 100644 --- a/docs/agents.md +++ b/docs/agents.md @@ -248,7 +248,7 @@ once per run inside the application scope and appended to `spec.warmers`. | Name | Signature | Notes | |---|---|---| | `ServiceError` | `ServiceError(detail=None, *, code=None, kind=None, params=None, public=None)` | subclass it and set `kind` / `code` / `public` as class attributes; `code` defaults to the snake_cased class name minus `Error` | -| `ErrorKind` | `INVALID`, `UNAUTHENTICATED`, `FORBIDDEN`, `NOT_FOUND`, `CONFLICT`, `PRECONDITION_FAILED`, `TOO_MANY_REQUESTS`, `DEADLINE_EXCEEDED`, `UNAVAILABLE`, `NOT_IMPLEMENTED`, `INTERNAL` | 400/401/403/404/409/412/429/504/503/501/500 over HTTP | +| `ErrorKind` | `INVALID`, `UNAUTHENTICATED`, `FORBIDDEN`, `NOT_FOUND`, `CONFLICT`, `PRECONDITION_FAILED`, `TOO_MANY_REQUESTS`, `DEADLINE_EXCEEDED`, `UNAVAILABLE`, `NOT_IMPLEMENTED`, `INTERNAL` | 400/401/403/404/409/400/429/504/503/501/500 over HTTP (each the `google.rpc.Code` mapping of the gRPC status) | | `ErrorInfo` | `kind`, `code`, `detail=None`, `params={}`, `public=True`, `status_override=None`, `headers=None`; `.http_status`; `ErrorInfo.from_service_error(exc)` | the normalized view every renderer takes | | `mask_private_error` | `mask_private_error(info) -> ErrorInfo` | `public=False` collapses to a generic `internal_error` 500 | | `INTERNAL_ERROR_CODE` | `"internal_error"` | the code every masked error renders as, on both transports | diff --git a/docs/concepts/errors.md b/docs/concepts/errors.md index 497c6ed..2f04209 100644 --- a/docs/concepts/errors.md +++ b/docs/concepts/errors.md @@ -60,23 +60,33 @@ except ServiceError as exc: | `FORBIDDEN` | 403 | `PERMISSION_DENIED` | | `NOT_FOUND` | 404 | `NOT_FOUND` | | `CONFLICT` | 409 | `ALREADY_EXISTS` | -| `PRECONDITION_FAILED` | 412 | `FAILED_PRECONDITION` | +| `PRECONDITION_FAILED` | 400 | `FAILED_PRECONDITION` | | `TOO_MANY_REQUESTS` | 429 | `RESOURCE_EXHAUSTED` | | `NOT_IMPLEMENTED` | 501 | `UNIMPLEMENTED` | | `UNAVAILABLE` | 503 | `UNAVAILABLE` | | `DEADLINE_EXCEEDED` | 504 | `DEADLINE_EXCEEDED` | | `INTERNAL` | 500 | `INTERNAL` | -Both columns are injective, so a status maps back to exactly one kind — which is what lets a -gateway translate between the two without guessing. +The gRPC column is injective, so a gRPC status maps back to exactly one kind. The HTTP column +follows from it: every HTTP status is the one `google.rpc.Code` assigns to that gRPC status, so a +client gets the same answer from the HTTP side as from grpc-gateway or Envoy's transcoder in front +of the gRPC side. -One row is worth stating out loud, because it has a plausible-looking alternative: `CONFLICT` -renders as `ALREADY_EXISTS` and not as `ABORTED`, although both mean 409 to a gRPC gateway. -`ABORTED` is the transaction-conflict code, and the gRPC contract tells clients to retry it at a -higher level — the wrong advice for "this order was already paid", which will never succeed on a -retry. `FAILED_PRECONDITION` is spoken for by `PRECONDITION_FAILED`. A client that has to tell a -duplicate key from a state conflict branches on `x-error-code`, which carries the exact code -either way, rather than on the status. +Two rows are worth stating out loud, because each has a plausible-looking alternative. + +`CONFLICT` renders as `ALREADY_EXISTS` and not as `ABORTED`, although both mean 409 to a gRPC +gateway. `ABORTED` is the transaction-conflict code, and the gRPC contract tells clients to retry +it at a higher level — the wrong advice for "this order was already paid", which will never +succeed on a retry. `FAILED_PRECONDITION` is spoken for by `PRECONDITION_FAILED`. + +`PRECONDITION_FAILED` renders as 400 and not as 412. RFC 9110 reserves 412 for conditional +request headers (`If-Match`, `If-Unmodified-Since`) evaluating to false, and clients, proxies and +SDKs read it as "your copy is stale: refetch and retry", the wrong advice for "this order is not +paid yet". A conditional request your own code rejects with `HTTPException(412)` still answers 412. + +So 400 means either `INVALID` or `PRECONDITION_FAILED`, and 409 is either a duplicate key or a +state conflict. A client that has to tell them apart branches on the error code (`code` in the +problem document, `x-error-code` over gRPC), which is exact either way, rather than on the status. ## What the client sees @@ -184,6 +194,25 @@ FastApiEntrypoint(error_renderer=ProblemDetailsRenderer(type_base="https://error # → "type": "https://errors.example.com/order_not_found" ``` +A renderer can also move one kind to another status. Clients written when `PRECONDITION_FAILED` +answered 412 can keep getting 412 while they migrate: + +```python +import dataclasses + +from servicewright import ErrorInfo, ErrorKind, ProblemDetailsRenderer, RenderedError + + +class Precondition412Renderer(ProblemDetailsRenderer): + def render(self, info: ErrorInfo) -> RenderedError: + if info.kind is ErrorKind.PRECONDITION_FAILED and info.status_override is None: + info = dataclasses.replace(info, status_override=412) + return super().render(info) + + +FastApiEntrypoint(error_renderer=Precondition412Renderer()) +``` + ### Rendering never fails Rendering an error is the client's last chance to learn what went wrong, so it may not be the diff --git a/servicewright/core/errors.py b/servicewright/core/errors.py index 8de3571..fc9ebb2 100644 --- a/servicewright/core/errors.py +++ b/servicewright/core/errors.py @@ -50,13 +50,19 @@ class ErrorKind(enum.StrEnum): INTERNAL = "internal" +# Each status is the one google.rpc.Code assigns to the kind's gRPC status, so a +# service answers the same over HTTP as grpc-gateway or Envoy's transcoder does +# in front of its gRPC side. That makes PRECONDITION_FAILED (FAILED_PRECONDITION) +# a 400 shared with INVALID, not 412: RFC 9110 keeps 412 for conditional request +# headers (If-Match and friends), and clients treat it as a stale ETag. Clients +# tell the two 400s apart by the error code, not the status. HTTP_STATUS_BY_KIND: dict[ErrorKind, int] = { ErrorKind.INVALID: 400, ErrorKind.UNAUTHENTICATED: 401, ErrorKind.FORBIDDEN: 403, ErrorKind.NOT_FOUND: 404, ErrorKind.CONFLICT: 409, - ErrorKind.PRECONDITION_FAILED: 412, + ErrorKind.PRECONDITION_FAILED: 400, ErrorKind.TOO_MANY_REQUESTS: 429, ErrorKind.DEADLINE_EXCEEDED: 504, ErrorKind.UNAVAILABLE: 503, diff --git a/tests/unit/test_error_parity.py b/tests/unit/test_error_parity.py index 0809e6e..fc7e458 100644 --- a/tests/unit/test_error_parity.py +++ b/tests/unit/test_error_parity.py @@ -38,6 +38,22 @@ SECRET = "dsn=postgres://user:pw@db/ledger" +# google.rpc.Code's HTTP mapping (google/rpc/code.proto): what grpc-gateway or +# Envoy's transcoder answers in front of a gRPC service. +HTTP_STATUS_BY_GRPC_STATUS = { + grpc.StatusCode.INVALID_ARGUMENT: 400, + grpc.StatusCode.UNAUTHENTICATED: 401, + grpc.StatusCode.PERMISSION_DENIED: 403, + grpc.StatusCode.NOT_FOUND: 404, + grpc.StatusCode.ALREADY_EXISTS: 409, + grpc.StatusCode.FAILED_PRECONDITION: 400, + grpc.StatusCode.RESOURCE_EXHAUSTED: 429, + grpc.StatusCode.DEADLINE_EXCEEDED: 504, + grpc.StatusCode.UNAVAILABLE: 503, + grpc.StatusCode.UNIMPLEMENTED: 501, + grpc.StatusCode.INTERNAL: 500, +} + class OrderNotFoundError(ServiceError): kind = ErrorKind.NOT_FOUND @@ -64,7 +80,7 @@ class Conclusion: def _kind_of(status: Any, table: Mapping[ErrorKind, Any]) -> ErrorKind: - """Reverse the transport's own mapping table; both are injective.""" + """Reverse the transport's own mapping table (HTTP 400 reverses to ``INVALID``).""" return next(kind for kind, mapped in table.items() if mapped == status) @@ -183,3 +199,8 @@ async def test__public_service_error__raised_over_either_transport__reaches_the_ assert conclusion.kind is ErrorKind.NOT_FOUND assert conclusion.code == "order_not_found" assert conclusion.detail == "no order with id 42" + + +@pytest.mark.parametrize("kind", list(ErrorKind)) +def test__error_kind__http_status__is_what_a_grpc_gateway_answers_for_its_grpc_status(kind: ErrorKind) -> None: + assert HTTP_STATUS_BY_KIND[kind] == HTTP_STATUS_BY_GRPC_STATUS[GRPC_STATUS_BY_KIND[kind]] diff --git a/tests/unit/test_fastapi.py b/tests/unit/test_fastapi.py index 5cdfc8f..5ce740d 100644 --- a/tests/unit/test_fastapi.py +++ b/tests/unit/test_fastapi.py @@ -505,6 +505,14 @@ async def http404() -> None: async def http500() -> None: raise HTTPException(status_code=500, detail="boom") + @router.put("/http-412") + async def http412() -> None: + raise HTTPException(status_code=412, detail="stale ETag") + + @router.post("/precondition") + async def precondition() -> None: + raise ServiceError("order 42 is not paid yet", code="order_not_paid", kind=ErrorKind.PRECONDITION_FAILED) + @router.get("/unhandled") async def unhandled() -> None: raise RuntimeError("kaboom") @@ -553,6 +561,20 @@ async def test__http_exception_handler__4xx__preserves_the_detail() -> None: assert body["detail"] == "not found" +async def test__http_exception_handler__412_from_a_conditional_request__keeps_412() -> None: + resp = (await _exc_client()).put("/http-412") + assert resp.status_code == 412 + assert resp.json()["detail"] == "stale ETag" + + +async def test__service_error_handler__precondition_failed__answers_400_not_412() -> None: + resp = (await _exc_client()).post("/precondition") + assert resp.status_code == 400 + body = resp.json() + assert body["status"] == 400 + assert body["code"] == "order_not_paid" + + async def test__http_exception_handler__5xx__masks_it_to_a_generic_internal_error() -> None: resp = (await _exc_client()).get("/http-500") assert resp.status_code == 500