From 1509fa47b894bb93f7beb7f0fbe5361dd5b6e141 Mon Sep 17 00:00:00 2001 From: Jorge Rivera Date: Thu, 22 Oct 2026 17:18:59 +0200 Subject: [PATCH 1/2] fix(presentation): match refusals to the code registry and bind routes A frame store that fails to answer, a FrameUnavailable with reason io, refuses service_unavailable (503, retryable) on every Presentation route. Every other FrameUnavailable reason refuses frame_unavailable at 404. Stored bytes that do not decode as a frame refuse missing_rendering, since no retry can load them. The render routes read each refusal's status from the code registry on presentation.render, which retires the hand-kept _RENDER_STATUS table. The Presentation surface binds create and revise to presentation.write and the two render routes to presentation.render. The Skills surface binds skills.list, skills.fetch and skills.resource. The five spec-kit pins move to 4f927c0, which maps the Presentation write refusals at the statuses the write routes serve and edit_conflict on presentation.render. tests/presentation_api/test_refusal_registry.py and tests/skills_api/test_refusal_registry.py check that every endpoint is bound and that every exemplar goes out at its registered status. --- docs/conformance-presentation-api-v1.md | 49 ++++++----- packages/dfo-data-tools/pyproject.toml | 8 +- pyproject.toml | 8 +- src/presentation_api/README.md | 8 +- src/presentation_api/app.py | 3 + src/presentation_api/codes.py | 6 +- src/presentation_api/doors/components.py | 25 +++++- src/presentation_api/doors/render.py | 29 +++--- src/presentation_api/refusals.py | 30 +++---- src/presentation_api/version.py | 2 +- src/presentation_store/frame_store.py | 6 +- src/skills_api/app.py | 3 + src/skills_api/doors/fetch.py | 3 +- src/skills_api/doors/list.py | 3 +- src/skills_api/doors/resource.py | 3 +- .../test_capability_version.py | 4 +- tests/presentation_api/test_frames_door.py | 6 +- .../presentation_api/test_refusal_registry.py | 88 +++++++++++++++++++ tests/presentation_store/test_frame_store.py | 12 +++ tests/skills_api/test_refusal_registry.py | 69 +++++++++++++++ uv.lock | 20 ++--- 21 files changed, 296 insertions(+), 89 deletions(-) create mode 100644 tests/presentation_api/test_refusal_registry.py create mode 100644 tests/skills_api/test_refusal_registry.py diff --git a/docs/conformance-presentation-api-v1.md b/docs/conformance-presentation-api-v1.md index 670148ce..465182d7 100644 --- a/docs/conformance-presentation-api-v1.md +++ b/docs/conformance-presentation-api-v1.md @@ -31,7 +31,7 @@ verified against `src/presentation_api` and the shared platform envelope it rend | C7b | POST /api/presentation/components/{component_id}/revisions/{revision}/render takes `{"frame": {spec_id, content_version}}` and answers what a component read answers for that revision, built from its stored intent on the named frame, with `resolved_payload.provenance.inputs` naming that frame and `provenance.refreshed_from` set to the revision. It appends no revision and pins the frame for the component. It admits only the caller holding `LIVE_REFRESH_PRINCIPAL` in the `DATASETS_PRINCIPAL_HEADER` header, checked before the path or body is validated | implemented | | | C7c | GET /api/presentation/components/{component_id}/revisions/{revision}/render/data?spec_id=&content_version= serves the C7a export of that revision rendered on the named frame, with the same `format` and `bundle` options, under the caller rule of C7b. It pins nothing | implemented | | | C7d | C7b and C7c accept the frame the revision is stored on, a dataset frame of the same `spec_id` and rendering, or a derived frame whose recipe has the stored frame's `normal_form_hash`. Any other frame refuses `render_target_mismatch`, 409 | implemented | | -| C7e | On C7b and C7c `binding_invalid` and `constraint_violation` answer 422, and `frame_unavailable`, `release_mismatch`, and `resource_exhausted` answer 409, the statuses the code registry maps for `presentation.render`. "Refusal code detail" below gives the statuses create and revise serve | implemented | | +| C7e | On C7b and C7c every refusal answers the status the code registry maps for its code on `presentation.render`: `binding_invalid` and `constraint_violation` answer 422, and `frame_unavailable`, `release_mismatch`, and `resource_exhausted` answer 409. A component with no current revision refuses `edit_conflict`, 409. "Refusal code detail" below gives the statuses create and revise serve | implemented | | ## Request shape @@ -99,7 +99,7 @@ alone. | write_refused | 403 | do_not_retry | implemented | | | request_invalid | 400 | headline correct_and_retry; wire `retryable` always false | implemented | | | resource_exhausted | 400 | do_not_retry | implemented | | -| frame_unavailable | 404 or 503 per cause (see below) | refresh_then_retry (expired, ops_unsupported) / do_not_retry (revoked, missing_rendering, digest_mismatch, quarantined, unknown) / retry_after_delay (io) | implemented | | +| frame_unavailable | 404 | refresh_then_retry (expired, ops_unsupported) / do_not_retry (revoked, missing_rendering, digest_mismatch, quarantined, unknown) | implemented | | | release_mismatch | 503 | retry_after_delay | implemented | | | service_unavailable | 503 | retry_after_delay | implemented | | | timeout | 504 | retry_after_delay | implemented | | @@ -108,31 +108,40 @@ alone. | refresh_not_permitted | 403 | do_not_retry | implemented | | | render_target_mismatch | 409 | do_not_retry | implemented | | -### frame_unavailable outcome by reason +### FrameUnavailable outcome by reason -| Reason | HTTP | retryable | -| ------------------ | ---- | --------- | -| expired | 404 | true | -| revoked | 404 | false | -| ops_unsupported | 404 | true | -| missing_rendering | 404 | false | -| digest_mismatch | 404 | false | -| quarantined | 404 | false | -| io | 503 | true | -| unknown | 404 | false | -| any other reason | 503 | true | +A frame store that fails to answer (`io`) refuses `service_unavailable`. A reason outside this table fails classification, and the platform default answers `internal_error`. + +| Reason | Code | HTTP | retryable | +| ----------------- | ------------------- | ---- | --------- | +| expired | frame_unavailable | 404 | true | +| revoked | frame_unavailable | 404 | false | +| ops_unsupported | frame_unavailable | 404 | true | +| missing_rendering | frame_unavailable | 404 | false | +| digest_mismatch | frame_unavailable | 404 | false | +| quarantined | frame_unavailable | 404 | false | +| io | service_unavailable | 503 | true | +| unknown | frame_unavailable | 404 | false | ## Registry binding `presentation_api.codes.PRESENTATION_CODE_CATALOGUE` union -`presentation_api.codes.PRESENTATION_SERVED_PLATFORM_CODES` — the seven codes this surface mints -itself, plus the six platform codes its own handlers, its own `validation_problem` path, or the -platform default put on the wire (`not_found`, `request_invalid`, `service_unavailable`, `timeout`, -`internal_error`, `unsupported_schema_version`) — is exactly the set of active spec-kit registry -entries whose mappings name a `presentation.*` endpoint (all thirteen map onto -`presentation.write`). `tests/platform/test_registry_binding.py`'s +`presentation_api.codes.PRESENTATION_SERVED_PLATFORM_CODES`, the nine codes this surface mints +itself plus the seven platform codes its handlers, its `validation_problem` path, the +live-refresh check, or the platform default put on the wire (`not_found`, `request_invalid`, +`service_unavailable`, `timeout`, `internal_error`, `unsupported_schema_version`, +`refresh_not_permitted`), is exactly the set of active spec-kit registry entries whose mappings +name a `presentation.*` endpoint. `tests/platform/test_registry_binding.py`'s `test_presentation_served_codes_match_registry` asserts this bijection and passes. `PRESENTATION_SERVED_PLATFORM_CODES` is what carries `C12`'s `unsupported_schema_version` into that comparison: it has no `CODE_RETRY_MODE` entry, since that table mirrors the baked catalogue manifest's `retry_modes` byte for byte, so `CODE_RETRY_MODE`'s own key set alone would leave the code out of the served set on both sides. + +The surface binds `presentation.write` to create and revise, and `presentation.render` to C7b +and C7c (`Surface.registry_endpoints`). The component read shares revise's path, so it is bound +with it. The frames, component data, release, and meta routes map no registry endpoint and serve +the write routes' statuses. `tests/presentation_api/test_refusal_registry.py` asserts that every +`presentation.*` endpoint is bound and that every exemplar goes out at the status the registry +gives its code: a render door exemplar on `presentation.render`, every other on +`presentation.write`. diff --git a/packages/dfo-data-tools/pyproject.toml b/packages/dfo-data-tools/pyproject.toml index e8f72624..bddbb736 100644 --- a/packages/dfo-data-tools/pyproject.toml +++ b/packages/dfo-data-tools/pyproject.toml @@ -86,10 +86,10 @@ graph-release = ["google-cloud-spanner>=3.57", "dfo-data-tools[coverage]"] [tool.uv.sources] # See the root pyproject.toml's dfo-spec-kit source comment for the # pin-consistency invariant this rev must satisfy. -dfo-spec-kit-frame-grammar = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "3616b64245c2db8af3187552e9e5de99b6d304d4", subdirectory = "python/frame-grammar" } -dfo-spec-kit-components = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "3616b64245c2db8af3187552e9e5de99b6d304d4", subdirectory = "python/dfo-spec-kit-components" } -dfo-spec-kit-provenance-docs = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "3616b64245c2db8af3187552e9e5de99b6d304d4", subdirectory = "python/provenance-docs" } -dfo-spec-kit-descriptions = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "3616b64245c2db8af3187552e9e5de99b6d304d4", subdirectory = "python/descriptions" } +dfo-spec-kit-frame-grammar = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "4f927c09040fc287f81fc5d81ce22eebe3caa2fd", subdirectory = "python/frame-grammar" } +dfo-spec-kit-components = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "4f927c09040fc287f81fc5d81ce22eebe3caa2fd", subdirectory = "python/dfo-spec-kit-components" } +dfo-spec-kit-provenance-docs = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "4f927c09040fc287f81fc5d81ce22eebe3caa2fd", subdirectory = "python/provenance-docs" } +dfo-spec-kit-descriptions = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "4f927c09040fc287f81fc5d81ce22eebe3caa2fd", subdirectory = "python/descriptions" } [build-system] requires = ["uv_build>=0.11,<0.12"] diff --git a/pyproject.toml b/pyproject.toml index 00bc3b1a..50bade59 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -236,12 +236,16 @@ dfo-data-tools = { workspace = true } # The registry refuses a part's `source_code` on search.resolve with # `source_code_not_held`, `source_code_ambiguous`, and # `source_code_shares_variable`. -# The pin is a commit of spec-kit's main. It registers +# The pin is spec-kit's main plus three commits that map binding_invalid, +# constraint_violation and resource_exhausted at 400, frame_unavailable at +# 404 and release_mismatch at 503 on presentation.write, and edit_conflict +# at 409 on presentation.render, and name every case frame_unavailable +# covers. Main registers # slot_default_not_applied and the three source-code refusals, and retires # starved_axis_binding_restored. # Follow-up: move all five to a real spec-kit tag once one is cut, so a # future force-push or GC on spec-kit's main can't silently break this lock. -dfo-spec-kit = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "3616b64245c2db8af3187552e9e5de99b6d304d4", subdirectory = "python/spec-kit" } +dfo-spec-kit = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "4f927c09040fc287f81fc5d81ce22eebe3caa2fd", subdirectory = "python/spec-kit" } [tool.pytest.ini_options] testpaths = ["tests"] diff --git a/src/presentation_api/README.md b/src/presentation_api/README.md index cc1e9655..c35dbdb4 100644 --- a/src/presentation_api/README.md +++ b/src/presentation_api/README.md @@ -398,13 +398,14 @@ The frame must be able to stand in for the frame the revision is stored on: Any other frame is refused `render_target_mismatch` (409, not retryable). That covers another dataset or rendering, a dataset frame for a card on a derived frame and the reverse, a version or recipe that is not on record, and a `note_narration`, which binds no frame. -The render routes serve the assembler's refusals at the statuses the code registry gives `presentation.render`, which differ from the write routes' for five codes: +The render routes serve each refusal at the status the code registry gives its code on `presentation.render`, which for `binding_invalid`, `constraint_violation`, `frame_unavailable`, `release_mismatch`, and `resource_exhausted` differs from the write routes' status: | Error Code | HTTP Status | When | | :----------------------- | :---------- | :------------------------------------------------------------------------------------------- | | `refresh_not_permitted` | 403 | The caller is not the live-refresh principal. | | `render_target_mismatch` | 409 | The frame may not stand in for the stored one. | | `not_found` | 404 | Unknown component or revision. On `/render/data`, also a `source_card`, which shows no data values. | +| `edit_conflict` | 409 | The component has no current revision. | | `request_invalid` | 400 | A malformed id, body, `format`, or `bundle`. | | `schema_invalid` | 400 | The stored intent fails this release's schema. | | `binding_invalid` | 422 | The stored intent does not bind the frame: a stored binding with no value left on the frame, or a single or facet value the frame lacks while it holds several. | @@ -412,6 +413,7 @@ The render routes serve the assembler's refusals at the statuses the code regist | `frame_unavailable` | 409 | The frame does not load or pin. `retryable` follows the cause, as on the write routes. | | `release_mismatch` | 409 | The revision was minted under a newer catalogue release than this deployment's. | | `resource_exhausted` | 409 | The drawing passes a resource maximum. | +| `service_unavailable` | 503 | A store, the frame store included, fails to answer, or the door is at capacity. | On these routes `retryable` is true only for a refusal the same request may pass later, one whose code's retry mode is `retry_after_delay` or `refresh_then_retry` (`codes.replay_retryable`). The caller replays a stored intent and cannot change it, so `binding_invalid`, `constraint_violation`, `schema_invalid`, and `request_invalid` answer `retryable: false`. @@ -433,7 +435,7 @@ The per-response `retryable` field is authoritative for every code. `retry_modes | `binding_invalid` | 400 | `correct_and_retry` | Unbound selectors or invalid values. Returns `alternatives`, `truncated`, and `issues`. Each alternative pairs a legal value with the label the frame resolved for it, falling back to the value itself when the frame names none. | | `constraint_violation` | 400 | `correct_and_retry` | Violates catalogue constraints. Returns `constraint_id`, `pointer`, and `detail`. `pointer` is expressed against the intent, for example `/scale`, `/range`, or `/change/basis`. | | `edit_conflict` | 409 | `refresh_then_retry` | The component, a requested revision, or on create the referenced composition vanished or was rewritten mid flight, raised by a scoped or unscoped `GET` as well as by writes. | -| `frame_unavailable` | 404 / 503 | Contextual | `unknown` (404, not retryable) names a `spec_id`/`content_version` pair that was never stored. `expired` (404, retryable) names a stored version that aged out. `ops_unsupported` (404, retryable) names an `ops_`/`ocv_` pair analysis-mcp minted but never published to the platform; publishing it is the retry that succeeds. `revoked` and the other permanent reasons (`missing_rendering`, `digest_mismatch`) return 404, not retryable. `quarantined` (404, not retryable) names a `spec_id` the spec registry holds in quarantine. `io` returns 503, retry after delay. | +| `frame_unavailable` | 404 | Contextual | `unknown` (404, not retryable) names a `spec_id`/`content_version` pair that was never stored. `expired` (404, retryable) names a stored version that aged out. `ops_unsupported` (404, retryable) names an `ops_`/`ocv_` pair analysis-mcp minted but never published to the platform; publishing it is the retry that succeeds. `revoked` and the other permanent reasons (`missing_rendering`, `digest_mismatch`) return 404, not retryable. `quarantined` (404, not retryable) names a `spec_id` the spec registry holds in quarantine. A frame store that fails to answer (`io`) refuses `service_unavailable` instead. | | `resource_exhausted` | 400 | `do_not_retry` | Selection exceeds allowed limits, including a minted component whose `component`, `provenance` and `description` subtrees serialise past `MAX_ENVELOPE_BYTES` (2 MiB, compact UTF-8 JSON), refused before any store write. Returns `limit_name` in `detail`. | | `release_mismatch` | 503 | `retry_after_delay` | The component's head carries a newer catalogue release than this deployment writes, which happens after a rollback and clears once the newer release is deployed again, or a stored payload has a shape the release's schema does not recognise, such as a bar_chart row migration 0006 has not rewritten. | | `request_invalid` | 400 | `correct_and_retry` | Retryable is always `false`. A malformed body field, an unparseable path identifier, or a non-positive `revision` query parameter. Also a create `write_token` reused with a different request. | @@ -442,7 +444,7 @@ The per-response `retryable` field is authoritative for every code. `retry_modes | `render_target_mismatch` | 409 | `do_not_retry` | A render route given a frame that may not stand in for the stored one. | | `not_found` | 404 | `do_not_retry` | Unknown component or composition id (`DraftNotFound`), or a revision number never minted for a revision-scoped `GET`. | | `write_refused` | 403 | `do_not_retry` | A revise without the component's write token, or a create into an existing composition without that composition's token, in `x-dfo-write-token`. Also a draft created before write tokens. `detail` ends with a fix: create a new component. | -| `service_unavailable` | 503 | `retry_after_delay` | Database failure or door thread pool backpressure exhaustion. | +| `service_unavailable` | 503 | `retry_after_delay` | Database failure, a frame store that fails to answer, or door thread pool backpressure exhaustion. | | `timeout` | 504 | `retry_after_delay` | Execution exceeded door timeout threshold. | | `internal_error` | 500 | `do_not_retry` | Permanent non-retryable store or system failure. | diff --git a/src/presentation_api/app.py b/src/presentation_api/app.py index ea634601..1455398b 100644 --- a/src/presentation_api/app.py +++ b/src/presentation_api/app.py @@ -128,5 +128,8 @@ def request_scope() -> Iterator[None]: exemplar for door in doors for exemplar in door.exception_exemplars ), lifespan=lifespan, + registry_endpoints={ + path: endpoint for door in doors for path, endpoint in door.registry_endpoints.items() + }, request_scope=request_scope, ) diff --git a/src/presentation_api/codes.py b/src/presentation_api/codes.py index 89c8613c..91d46749 100644 --- a/src/presentation_api/codes.py +++ b/src/presentation_api/codes.py @@ -19,12 +19,12 @@ intent and cannot correct it, so there `retryable` is true only when the same request may succeed later: `replay_retryable` keeps an occurrence's value for a `retry_after_delay` or `refresh_then_retry` code and answers false for every other mode. -Frame unavailability returns 404 or transient 503. Release mismatch and service unavailability -return 503. Timeout returns 504, and internal errors return 500. +Frame unavailability returns 404, and a frame store that fails to answer returns +`service_unavailable`. Release mismatch and service unavailability return 503. Timeout returns 504, and internal errors return 500. Missing-field `schema_invalid`, `binding_invalid`, and `constraint_violation` use `correct_and_retry`. Illegal fields use `do_not_retry`. Conflicts and some frame refusals need a -refresh. Transient frame, release, service, and timeout failures retry after a delay. `not_found`, +refresh. Transient release, service, and timeout failures retry after a delay. `not_found`, `write_refused`, `resource_exhausted`, `refresh_not_permitted`, `render_target_mismatch`, and `internal_error` use `do_not_retry`. `CODE_RETRY_MODE` mirrors the release manifest. diff --git a/src/presentation_api/doors/components.py b/src/presentation_api/doors/components.py index 3739e8b3..d5a01bc9 100644 --- a/src/presentation_api/doors/components.py +++ b/src/presentation_api/doors/components.py @@ -69,8 +69,11 @@ classify_component_data_exception, ) from presentation_api.doors.render import ( + RENDER_DATA_PATH, + RENDER_ENDPOINT, RENDER_EXCEPTION_EXEMPLARS, RENDER_EXCEPTION_TYPES, + RENDER_PATH, build_render_router, classify_render_exception, ) @@ -97,6 +100,10 @@ # composition, sends it back. WRITE_TOKEN_HEADER = "x-dfo-write-token" +COMPONENTS_PATH = "/api/presentation/components" +COMPONENT_PATH = f"{COMPONENTS_PATH}/{{component_id}}" +WRITE_ENDPOINT = "presentation.write" + class PresentationBackpressure(Exception): """Raised when the shared components semaphore is full.""" @@ -403,7 +410,7 @@ async def _run[R: Response](job: Callable[[], R], /) -> R: router = APIRouter() @router.post( - "/api/presentation/components", + COMPONENTS_PATH, status_code=201, responses={201: {"model": CreateReplyResponse}}, ) @@ -428,7 +435,7 @@ def job() -> JSONResponse: return await _run(job) @router.patch( - "/api/presentation/components/{component_id}", + COMPONENT_PATH, responses={200: {"model": WriteReplyResponse}}, ) async def revise_component( @@ -454,7 +461,7 @@ def job() -> JSONResponse: return await _run(job) @router.get( - "/api/presentation/components/{component_id}", + COMPONENT_PATH, responses={200: {"model": ComponentReadResponse}}, ) async def read_component( @@ -479,7 +486,9 @@ def job() -> JSONResponse: build_render_router( deps=deps, run=_run, - classify=classify_components_exception, + classify=combine_classifiers( + classify_components_exception, classify_component_data_exception + ), live_refresh=live_refresh, ).routes ) @@ -491,6 +500,14 @@ def job() -> JSONResponse: classify_component_data_exception, classify_render_exception, ), + # The component read shares the revise route's path, so it runs with + # `presentation.write` bound and serves the write routes' statuses. + registry_endpoints={ + COMPONENTS_PATH: WRITE_ENDPOINT, + COMPONENT_PATH: WRITE_ENDPOINT, + RENDER_PATH: RENDER_ENDPOINT, + RENDER_DATA_PATH: RENDER_ENDPOINT, + }, exception_types=( PathParamInvalid, CreateRequestInvalid, diff --git a/src/presentation_api/doors/render.py b/src/presentation_api/doors/render.py index f4312e1c..16ddf524 100644 --- a/src/presentation_api/doors/render.py +++ b/src/presentation_api/doors/render.py @@ -36,10 +36,11 @@ the rule. A `note_narration` binds no frame, so no target can stand in for one, and it refuses `render_target_mismatch` too. -A refusal of the render carries the status the code registry gives its code on -`presentation.render`, which for five codes differs from the status the write routes serve: -`binding_invalid` and `constraint_violation` 422, and `frame_unavailable`, `release_mismatch`, -and `resource_exhausted` 409. The caller replays a stored intent it cannot correct, so +A refusal of the render goes out at the status the code registry gives its code on +`presentation.render` (`RENDER_ENDPOINT`), read through `dfo_platform.codes.registered_status`. +For five codes it differs from the status the write routes serve: `binding_invalid` and +`constraint_violation` answer 422, and `frame_unavailable`, `release_mismatch`, and +`resource_exhausted` 409. The caller replays a stored intent it cannot correct, so `retryable` is the write routes' value read through `presentation_api.codes.replay_retryable`: true only for a `retry_after_delay` or `refresh_then_retry` code whose occurrence is retryable. """ @@ -72,6 +73,7 @@ ) from datasets_api.recipe_store import RecipeReader from dfo_platform import ProblemSpec +from dfo_platform.codes import bound_registry_endpoint, registered_status from presentation_api.codes import replay_retryable, retryable_for from presentation_api.doors.component_data import COMPONENT_DATA_MEDIA_TYPES from presentation_api.models import ComponentReadResponse @@ -94,14 +96,7 @@ # name decides nothing. _NO_PRINCIPAL = LiveRefreshPrincipal(header="x-dfo-principal", principal="") -# The `presentation.render` status of each assembler refusal the write routes serve at another. -_RENDER_STATUS: Mapping[str, int] = { - "binding_invalid": 422, - "constraint_violation": 422, - "frame_unavailable": 409, - "release_mismatch": 409, - "resource_exhausted": 409, -} +RENDER_ENDPOINT = "presentation.render" class RenderTargetMismatch(Exception): @@ -272,9 +267,9 @@ def build_render_router( ) -> APIRouter: """The two render routes, run through the components door's `run` on its `deps`. - `classify` is the components door's classifier, whose problem for a refusal the routes - restate at its `presentation.render` status and replay retryability. `live_refresh=None` - refuses every caller. + `classify` is the components door's classifier for component and component data refusals, + whose problem the routes restate at its `presentation.render` status and replay + retryability. `live_refresh=None` refuses every caller. """ principal = live_refresh if live_refresh is not None else _NO_PRINCIPAL router = APIRouter(route_class=principal.route_class()) @@ -287,9 +282,11 @@ def restated() -> Iterator[None]: spec = classify(exc) or classify_render_exception(exc) if spec is None: raise + with bound_registry_endpoint(RENDER_ENDPOINT): + status = registered_status(spec.code) raise RenderRefused( spec._replace( - status=_RENDER_STATUS.get(spec.code, spec.status), + status=status, retryable=replay_retryable(spec.code, retryable=spec.retryable), ) ) from exc diff --git a/src/presentation_api/refusals.py b/src/presentation_api/refusals.py index 00b9c0f6..5e9d0cd2 100644 --- a/src/presentation_api/refusals.py +++ b/src/presentation_api/refusals.py @@ -47,25 +47,25 @@ def binding_invalid_spec(exc: BindingInvalid) -> ProblemSpec: ) -# `expired` and `ops_unsupported` can succeed after republishing. Revoked, missing, digest -# mismatch, quarantined, and unknown refs are permanent. I/O maps to retryable 503. -_FRAME_UNAVAILABLE_OUTCOME: dict[str, tuple[int, bool]] = { - "expired": (404, True), - "revoked": (404, False), - "ops_unsupported": (404, True), - "missing_rendering": (404, False), - "digest_mismatch": (404, False), - "quarantined": (404, False), - "io": (503, True), - "unknown": (404, False), +# The code, status, and retryability each `FrameUnavailable` reason refuses with. `expired` and +# `ops_unsupported` can succeed after republishing. Revoked, missing, digest mismatch, +# quarantined, and unknown refs are permanent. A store that fails to answer (`io`) is +# `service_unavailable`. +_FRAME_UNAVAILABLE_OUTCOME: dict[str, tuple[str, int, bool]] = { + "expired": ("frame_unavailable", 404, True), + "revoked": ("frame_unavailable", 404, False), + "ops_unsupported": ("frame_unavailable", 404, True), + "missing_rendering": ("frame_unavailable", 404, False), + "digest_mismatch": ("frame_unavailable", 404, False), + "quarantined": ("frame_unavailable", 404, False), + "io": ("service_unavailable", 503, True), + "unknown": ("frame_unavailable", 404, False), } def frame_unavailable_spec(exc: FrameUnavailable) -> ProblemSpec: - status, retryable = _FRAME_UNAVAILABLE_OUTCOME.get(exc.reason, (503, True)) - return ProblemSpec( - status=status, code="frame_unavailable", detail=exc.detail, retryable=retryable - ) + code, status, retryable = _FRAME_UNAVAILABLE_OUTCOME[exc.reason] + return ProblemSpec(status=status, code=code, detail=exc.detail, retryable=retryable) def request_validation_spec(exc: BaseException) -> ProblemSpec | None: diff --git a/src/presentation_api/version.py b/src/presentation_api/version.py index 0ef599e0..5b0f8caf 100644 --- a/src/presentation_api/version.py +++ b/src/presentation_api/version.py @@ -72,7 +72,7 @@ def compute_presentation_capability_version( release: CatalogueRelease, code_catalogue: CodeCatalogue = PRESENTATION_CODE_CATALOGUE, retry_modes: Mapping[str, str] = CODE_RETRY_MODE, - frame_unavailable_outcome: Mapping[str, tuple[int, bool]] = _FRAME_UNAVAILABLE_OUTCOME, + frame_unavailable_outcome: Mapping[str, tuple[str, int, bool]] = _FRAME_UNAVAILABLE_OUTCOME, ) -> str: """Return the capability hash for `release` and the supplied refusal data.""" refusal_exemplars = contextvars.copy_context().run( diff --git a/src/presentation_store/frame_store.py b/src/presentation_store/frame_store.py index 2ca8a6fa..dc4b9dcc 100644 --- a/src/presentation_store/frame_store.py +++ b/src/presentation_store/frame_store.py @@ -368,8 +368,8 @@ def _decode_frame( """The frame1 document `data` holds and the bytes the cache counts for it: a frame1 document parsed, at its byte length, or a canonical frame built into one, at `_document_bytes`. A canonical frame whose Parquet footer counts more than `max_rows` rows - refuses `missing_rendering` before any row is decoded. Bytes that do not decode refuse - `io`.""" + refuses `missing_rendering` before any row is decoded. Bytes that do not decode never will, + so they refuse `missing_rendering` too.""" try: if location.media_type == FRAME1_MEDIA_TYPE: return cast("Mapping[str, object]", json.loads(data)), len(data) @@ -379,7 +379,7 @@ def _decode_frame( doc = canonical_frame_document(read_canonical_frame_table(data)) except (ValueError, pa.ArrowException) as exc: raise FrameUnavailable( - reason="io", + reason="missing_rendering", detail=( f"artifact_digest {location.artifact_digest!r} did not decode as a frame: " f"{type(exc).__name__}" diff --git a/src/skills_api/app.py b/src/skills_api/app.py index a4db8796..ee9eb36c 100644 --- a/src/skills_api/app.py +++ b/src/skills_api/app.py @@ -81,4 +81,7 @@ async def lifespan(_app: FastAPI): exemplar for door in doors for exemplar in door.exception_exemplars ), lifespan=lifespan, + registry_endpoints={ + path: endpoint for door in doors for path, endpoint in door.registry_endpoints.items() + }, ) diff --git a/src/skills_api/doors/fetch.py b/src/skills_api/doors/fetch.py index cef09dba..eee3cd65 100644 --- a/src/skills_api/doors/fetch.py +++ b/src/skills_api/doors/fetch.py @@ -9,7 +9,7 @@ from fastapi import APIRouter, Request -from dfo_platform.doors import DoorContribution +from dfo_platform.doors import DoorContribution, route_registry_endpoints from skills_api.deps import ( CACHE_CONTROL, SkillIdTooLong, @@ -61,6 +61,7 @@ def build_fetch_door() -> DoorContribution: ) return DoorContribution( router=router, + registry_endpoints=route_registry_endpoints(router, "skills.fetch"), classify=classify_skill_lookup_exception, exception_types=(SkillIdTooLong, SkillNotFound, SkillsIndexNotBuilt), exception_exemplars=( diff --git a/src/skills_api/doors/list.py b/src/skills_api/doors/list.py index 8ccf3f7f..6fc1fa20 100644 --- a/src/skills_api/doors/list.py +++ b/src/skills_api/doors/list.py @@ -13,7 +13,7 @@ from fastapi import APIRouter, Request from dfo_platform import ProblemSpec -from dfo_platform.doors import DoorContribution +from dfo_platform.doors import DoorContribution, route_registry_endpoints from skills_api.deps import ( CACHE_CONTROL, INDEX_NOT_BUILT_PROBLEM, @@ -60,6 +60,7 @@ def build_list_door() -> DoorContribution: ) return DoorContribution( router=router, + registry_endpoints=route_registry_endpoints(router, "skills.list"), classify=classify_list_exception, exception_types=(SkillsIndexNotBuilt,), exception_exemplars=(SkillsIndexNotBuilt(),), diff --git a/src/skills_api/doors/resource.py b/src/skills_api/doors/resource.py index f87f6370..a56751f9 100644 --- a/src/skills_api/doors/resource.py +++ b/src/skills_api/doors/resource.py @@ -12,7 +12,7 @@ from fastapi import APIRouter, Request from dfo_platform import ProblemSpec -from dfo_platform.doors import DoorContribution +from dfo_platform.doors import DoorContribution, route_registry_endpoints from skills_api.deps import ( CACHE_CONTROL, SkillIdTooLong, @@ -128,6 +128,7 @@ def build_resource_door() -> DoorContribution: ) return DoorContribution( router=router, + registry_endpoints=route_registry_endpoints(router, "skills.resource"), classify=classify_resource_exception, exception_types=( SkillIdTooLong, diff --git a/tests/presentation_api/test_capability_version.py b/tests/presentation_api/test_capability_version.py index 49e34d55..1526f347 100644 --- a/tests/presentation_api/test_capability_version.py +++ b/tests/presentation_api/test_capability_version.py @@ -119,8 +119,8 @@ def test_frame_unavailable_outcome_change_moves_the_token(release: CatalogueRele baseline = version.compute_presentation_capability_version(release=release) mutated_outcome = dict(version._FRAME_UNAVAILABLE_OUTCOME) reason = next(iter(sorted(mutated_outcome))) - status, retryable = mutated_outcome[reason] - mutated_outcome[reason] = (status, not retryable) + code, status, retryable = mutated_outcome[reason] + mutated_outcome[reason] = (code, status, not retryable) perturbed = version.compute_presentation_capability_version( release=release, frame_unavailable_outcome=mutated_outcome diff --git a/tests/presentation_api/test_frames_door.py b/tests/presentation_api/test_frames_door.py index aba6db6d..e3e670e8 100644 --- a/tests/presentation_api/test_frames_door.py +++ b/tests/presentation_api/test_frames_door.py @@ -113,15 +113,15 @@ def test_an_unknown_dimension_is_binding_invalid(client: TestClient, frame_ref: assert resp.json()["code"] == "binding_invalid" -def test_an_unregistered_frame_is_frame_unavailable(client: TestClient): - # The fake maps an unregistered frame to retryable 503 `io`. +def test_a_frame_store_that_fails_to_answer_is_service_unavailable(client: TestClient): + # The fake refuses an unregistered frame with reason `io`. resp = client.get( "/api/presentation/frames/spec_00000000000000000000000000000000/" "cv_00000000000000000000000000000000", ) assert resp.status_code == 503 body = resp.json() - assert body["code"] == "frame_unavailable" + assert body["code"] == "service_unavailable" assert body["retryable"] is True diff --git a/tests/presentation_api/test_refusal_registry.py b/tests/presentation_api/test_refusal_registry.py new file mode 100644 index 00000000..bd1ba55c --- /dev/null +++ b/tests/presentation_api/test_refusal_registry.py @@ -0,0 +1,88 @@ +"""Every Presentation route that serves a code registry endpoint binds it, and every exemplar goes +out at the status the registry gives its code on the endpoint whose routes raise it: a render +door exemplar on `presentation.render`, and every other exemplar on `presentation.write`. + +The registry maps no endpoint onto Presentation's read routes. They share the write routes' +classifiers, so they serve the write routes' statuses. +""" + +from __future__ import annotations + +from dfo_spec_kit.registry import REGISTRY + +from assembler.fakes import FakeReleaseRegistry +from dfo_platform.codes import bound_registry_endpoint +from presentation_api.app import presentation_surface +from presentation_api.doors.render import RENDER_EXCEPTION_EXEMPLARS + +_WRITE = "presentation.write" +_RENDER = "presentation.render" + +# Routes that serve no registry endpoint. The component read shares the revise route's path, +# so it is bound with it. +_UNBOUND_PATHS = frozenset( + { + "/api/presentation/healthz", + "/api/presentation/capabilities", + "/api/presentation/release", + "/api/presentation/frames/{spec_id}/{content_version}", + "/api/presentation/components/{component_id}/revisions/{revision}/data", + } +) + + +def _statuses(code: str, endpoint: str) -> set[int]: + """The statuses the registry gives `code`'s problem details on `endpoint`.""" + entry = REGISTRY[code] if code in REGISTRY else None # noqa: SIM401 -- CodeRegistry has no .get() + return { + mapping.http_status + for mapping in (entry.mappings if entry is not None else ()) + if mapping.channel == "problem_details" + and mapping.http_status is not None + and mapping.endpoint == endpoint + } + + +def test_every_presentation_route_binds_the_registry_endpoint_it_serves( + release_registry: FakeReleaseRegistry, +): + """The surface binds every Presentation endpoint the registry maps, each to a route it + serves, and leaves only the routes in `_UNBOUND_PATHS` unbound.""" + surface = presentation_surface(release_registry=release_registry) + registered = { + mapping.endpoint + for entry in REGISTRY.entries + for mapping in entry.mappings + if mapping.endpoint.startswith("presentation.") + } + routed = {getattr(route, "path", None) for route in surface.router.routes} + + assert set(surface.registry_endpoints.values()) == registered + assert routed - set(surface.registry_endpoints) == _UNBOUND_PATHS + + +def test_every_presentation_exemplar_goes_out_at_its_registered_status( + release_registry: FakeReleaseRegistry, +): + """Each exemplar, classified with its routes' endpoint bound, goes out at the one status the + registry gives its code there, and its code is mapped on that endpoint.""" + surface = presentation_surface(release_registry=release_registry) + render_exemplars = {id(exemplar) for exemplar in RENDER_EXCEPTION_EXEMPLARS} + disagreements: set[tuple[str, str, str, int]] = set() + unmapped: set[tuple[str, str, str]] = set() + checked = 0 + for exemplar in surface.exception_exemplars: + endpoint = _RENDER if id(exemplar) in render_exemplars else _WRITE + with bound_registry_endpoint(endpoint): + spec = surface.classify(exemplar) + if spec is None: + continue + checked += 1 + statuses = _statuses(spec.code, endpoint) + if not statuses: + unmapped.add((type(exemplar).__qualname__, endpoint, spec.code)) + elif statuses != {spec.status}: + disagreements.add((type(exemplar).__qualname__, endpoint, spec.code, spec.status)) + assert checked > len(RENDER_EXCEPTION_EXEMPLARS) + assert sorted(disagreements) == [] + assert sorted(unmapped) == [] diff --git a/tests/presentation_store/test_frame_store.py b/tests/presentation_store/test_frame_store.py index edc296f5..09cbafca 100644 --- a/tests/presentation_store/test_frame_store.py +++ b/tests/presentation_store/test_frame_store.py @@ -739,6 +739,18 @@ def test_load_refuses_a_canonical_frame_over_the_row_bound_before_fetching_it(): assert [call for call in artifacts.calls if call[0] == "get"] == [] +def test_load_refuses_a_canonical_frame_whose_bytes_do_not_decode(): + """Stored bytes that match their digest and do not decode as a frame never load, so the + load refuses `missing_rendering`.""" + snapshots, artifacts, frame_ref = _canonical_frame_version(b"not a parquet file") + store = _store(snapshots=snapshots, artifacts=artifacts) + + with pytest.raises(FrameUnavailable) as excinfo: + store.load(frame_ref=frame_ref) + + assert excinfo.value.reason == "missing_rendering" + + def test_load_decodes_a_canonical_frame_at_the_row_bound(): snapshots, artifacts, frame_ref, frame = _canonical_frame_fixture() store = _store(snapshots=snapshots, artifacts=artifacts, frame_max_rows=frame.table.num_rows) diff --git a/tests/skills_api/test_refusal_registry.py b/tests/skills_api/test_refusal_registry.py new file mode 100644 index 00000000..964e2410 --- /dev/null +++ b/tests/skills_api/test_refusal_registry.py @@ -0,0 +1,69 @@ +"""Every Skills route that refuses binds the code registry endpoint it serves, and every exemplar +the Skills surface declares goes out at the status the registry gives its code on each Skills +endpoint that maps it. +""" + +from __future__ import annotations + +from dfo_spec_kit.registry import REGISTRY + +from dfo_platform.codes import bound_registry_endpoint +from skills_api.app import skills_surface + +# Routes that serve no registry endpoint: they refuse only with platform codes. +_UNBOUND_PATHS = frozenset({"/api/skills/healthz"}) + + +def _statuses(code: str, endpoint: str) -> set[int]: + """The statuses the registry gives `code`'s problem details on `endpoint`.""" + entry = REGISTRY[code] if code in REGISTRY else None # noqa: SIM401 -- CodeRegistry has no .get() + return { + mapping.http_status + for mapping in (entry.mappings if entry is not None else ()) + if mapping.channel == "problem_details" + and mapping.http_status is not None + and mapping.endpoint == endpoint + } + + +def test_every_skills_route_binds_the_registry_endpoint_it_serves(): + """The surface binds every Skills endpoint the registry maps, each to a route it serves, and + leaves only the routes in `_UNBOUND_PATHS` unbound.""" + surface = skills_surface(skills_index=object()) + registered = { + mapping.endpoint + for entry in REGISTRY.entries + for mapping in entry.mappings + if mapping.endpoint.startswith("skills.") + } + routed = {getattr(route, "path", None) for route in surface.router.routes} + + assert set(surface.registry_endpoints.values()) == registered + assert routed - set(surface.registry_endpoints) == _UNBOUND_PATHS + + +def test_every_skills_exemplar_goes_out_at_its_registered_status(): + """Each exemplar, classified with each Skills endpoint bound, goes out at the one status the + registry gives its code there. Its code is mapped on at least one Skills endpoint.""" + surface = skills_surface(skills_index=object()) + endpoints = sorted(set(surface.registry_endpoints.values())) + assert endpoints + disagreements: set[tuple[str, str, str, int]] = set() + unmapped: set[str] = set() + for exemplar in surface.exception_exemplars: + mapped = False + for endpoint in endpoints: + with bound_registry_endpoint(endpoint): + spec = surface.classify(exemplar) + if spec is None: + continue + statuses = _statuses(spec.code, endpoint) + if not statuses: + continue + mapped = True + if statuses != {spec.status}: + disagreements.add((type(exemplar).__qualname__, endpoint, spec.code, spec.status)) + if not mapped: + unmapped.add(type(exemplar).__qualname__) + assert sorted(disagreements) == [] + assert sorted(unmapped) == [] diff --git a/uv.lock b/uv.lock index 68e4c0bd..74b598c1 100644 --- a/uv.lock +++ b/uv.lock @@ -720,10 +720,10 @@ requires-dist = [ { name = "dfo-data-tools", extras = ["generation"], marker = "extra == 'resolve'", editable = "packages/dfo-data-tools" }, { name = "dfo-data-tools", extras = ["sdmx"], marker = "extra == 'coverage'", editable = "packages/dfo-data-tools" }, { name = "dfo-data-tools", extras = ["sdmx"], marker = "extra == 'formats'", editable = "packages/dfo-data-tools" }, - { name = "dfo-spec-kit-components", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdfo-spec-kit-components&rev=3616b64245c2db8af3187552e9e5de99b6d304d4" }, - { name = "dfo-spec-kit-descriptions", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdescriptions&rev=3616b64245c2db8af3187552e9e5de99b6d304d4" }, - { name = "dfo-spec-kit-frame-grammar", extras = ["arrow"], git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=3616b64245c2db8af3187552e9e5de99b6d304d4" }, - { name = "dfo-spec-kit-provenance-docs", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fprovenance-docs&rev=3616b64245c2db8af3187552e9e5de99b6d304d4" }, + { name = "dfo-spec-kit-components", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdfo-spec-kit-components&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd" }, + { name = "dfo-spec-kit-descriptions", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdescriptions&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd" }, + { name = "dfo-spec-kit-frame-grammar", extras = ["arrow"], git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd" }, + { name = "dfo-spec-kit-provenance-docs", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fprovenance-docs&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd" }, { name = "fastapi", marker = "extra == 'surface'", specifier = ">=0.138" }, { name = "google-auth", extras = ["requests"], marker = "extra == 'generation'", specifier = ">=2.55" }, { name = "google-cloud-bigquery", marker = "extra == 'coverage'", specifier = ">=3.27" }, @@ -748,7 +748,7 @@ provides-extras = ["sdmx", "formats", "surface", "resolve", "generation", "cover [[package]] name = "dfo-spec-kit" version = "0.2.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=3616b64245c2db8af3187552e9e5de99b6d304d4#3616b64245c2db8af3187552e9e5de99b6d304d4" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd#4f927c09040fc287f81fc5d81ce22eebe3caa2fd" } dependencies = [ { name = "rfc8785" }, ] @@ -756,7 +756,7 @@ dependencies = [ [[package]] name = "dfo-spec-kit-components" version = "0.1.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdfo-spec-kit-components&rev=3616b64245c2db8af3187552e9e5de99b6d304d4#3616b64245c2db8af3187552e9e5de99b6d304d4" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdfo-spec-kit-components&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd#4f927c09040fc287f81fc5d81ce22eebe3caa2fd" } dependencies = [ { name = "dfo-spec-kit-descriptions" }, ] @@ -764,7 +764,7 @@ dependencies = [ [[package]] name = "dfo-spec-kit-descriptions" version = "0.1.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdescriptions&rev=3616b64245c2db8af3187552e9e5de99b6d304d4#3616b64245c2db8af3187552e9e5de99b6d304d4" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdescriptions&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd#4f927c09040fc287f81fc5d81ce22eebe3caa2fd" } dependencies = [ { name = "rfc8785" }, ] @@ -772,7 +772,7 @@ dependencies = [ [[package]] name = "dfo-spec-kit-frame-grammar" version = "0.1.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=3616b64245c2db8af3187552e9e5de99b6d304d4#3616b64245c2db8af3187552e9e5de99b6d304d4" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd#4f927c09040fc287f81fc5d81ce22eebe3caa2fd" } dependencies = [ { name = "dfo-spec-kit" }, { name = "pyarrow" }, @@ -782,7 +782,7 @@ dependencies = [ [[package]] name = "dfo-spec-kit-provenance-docs" version = "0.1.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fprovenance-docs&rev=3616b64245c2db8af3187552e9e5de99b6d304d4#3616b64245c2db8af3187552e9e5de99b6d304d4" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fprovenance-docs&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd#4f927c09040fc287f81fc5d81ce22eebe3caa2fd" } dependencies = [ { name = "markdown-it-py" }, ] @@ -2927,7 +2927,7 @@ requires-dist = [ { name = "dfo-data-tools", extras = ["resolve", "sdmx", "coverage"], marker = "extra == 'graph'", editable = "packages/dfo-data-tools" }, { name = "dfo-data-tools", extras = ["sdmx"], marker = "extra == 'sdmx'", editable = "packages/dfo-data-tools" }, { name = "dfo-data-tools", extras = ["surface", "resolve", "coverage", "sdmx"], marker = "extra == 'engine'", editable = "packages/dfo-data-tools" }, - { name = "dfo-spec-kit", marker = "extra == 'engine'", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=3616b64245c2db8af3187552e9e5de99b6d304d4" }, + { name = "dfo-spec-kit", marker = "extra == 'engine'", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=4f927c09040fc287f81fc5d81ce22eebe3caa2fd" }, { name = "fastapi", marker = "extra == 'api'", specifier = ">=0.138" }, { name = "google-auth", marker = "extra == 'datasets'", specifier = ">=2.34" }, { name = "google-auth", extras = ["requests"], marker = "extra == 'eval'", specifier = ">=2.34" }, From 749b38494fcf569f008f7aa5b777b5dc610fcd40 Mon Sep 17 00:00:00 2001 From: Jorge Rivera Date: Sat, 31 Oct 2026 10:12:16 +0100 Subject: [PATCH 2/2] fix(presentation): read every status from the registry and bind every route by method --- docs/conformance-presentation-api-v1.md | 49 ++-- packages/dfo-analysis-engine/pyproject.toml | 4 +- packages/dfo-data-tools/pyproject.toml | 8 +- .../dfo-data-tools/src/assembler/ports.py | 29 ++- .../dfo-data-tools/src/dfo_platform/app.py | 14 +- .../dfo-data-tools/src/dfo_platform/doors.py | 21 +- .../src/dfo_platform/problem.py | 8 +- pyproject.toml | 12 +- src/datasets_api/app.py | 2 +- src/datasets_api/doors/definition.py | 7 +- src/datasets_api/doors/derived.py | 4 +- src/datasets_api/doors/exports.py | 4 +- src/datasets_api/doors/meta.py | 4 +- src/graph_api/app.py | 2 +- src/graph_api/doors/members.py | 4 +- src/graph_api/doors/node.py | 4 +- src/graph_api/doors/sources.py | 2 +- src/presentation_api/README.md | 12 +- src/presentation_api/app.py | 2 +- src/presentation_api/codes.py | 94 ++++++-- src/presentation_api/doors/component_data.py | 8 +- src/presentation_api/doors/components.py | 103 ++++---- src/presentation_api/doors/frames.py | 15 +- src/presentation_api/doors/render.py | 120 ++++------ src/presentation_api/doors/shares.py | 69 ++++-- src/presentation_api/newest_frames.py | 8 +- src/presentation_api/refusals.py | 79 ++++--- src/presentation_api/version.py | 19 +- src/presentation_store/README.md | 2 +- src/presentation_store/frame_store.py | 38 ++- src/search/engine/app.py | 4 +- src/skills_api/app.py | 2 +- tests/code_registry.py | 53 +++++ tests/contract/test_client_contract.py | 2 +- tests/datasets_api/test_refusal_registry.py | 29 +-- tests/engine/test_refusal_registry.py | 27 +-- tests/graph_api/test_refusal_registry.py | 23 +- tests/platform/test_registry_binding.py | 19 +- tests/platform/test_registry_endpoint.py | 17 +- .../test_capability_version.py | 19 +- tests/presentation_api/test_frames_door.py | 8 +- .../presentation_api/test_refusal_registry.py | 219 +++++++++++------- tests/presentation_api/test_render_door.py | 7 + .../presentation_api/test_share_data_door.py | 13 +- tests/presentation_store/test_frame_store.py | 81 ++++++- tests/skills_api/test_refusal_registry.py | 61 ++--- uv.lock | 28 +-- 47 files changed, 833 insertions(+), 526 deletions(-) create mode 100644 tests/code_registry.py diff --git a/docs/conformance-presentation-api-v1.md b/docs/conformance-presentation-api-v1.md index c2f67262..52352a24 100644 --- a/docs/conformance-presentation-api-v1.md +++ b/docs/conformance-presentation-api-v1.md @@ -31,7 +31,7 @@ verified against `src/presentation_api` and the shared platform envelope it rend | C7b | POST /api/presentation/components/{component_id}/revisions/{revision}/render takes `{"frame": {spec_id, content_version}}` and answers what a component read answers for that revision, built from its stored intent on the named frame, with `resolved_payload.provenance.inputs` naming that frame and `provenance.refreshed_from` set to the revision. It appends no revision and pins nothing, so the named frame keeps its retention. It admits only the caller holding `LIVE_REFRESH_PRINCIPAL` in the `DATASETS_PRINCIPAL_HEADER` header, checked before the path or body is validated | implemented | | | C7c | GET /api/presentation/components/{component_id}/revisions/{revision}/render/data?spec_id=&content_version= serves the C7a export of that revision rendered on the named frame, with the same `format` and `bundle` options, under the caller rule of C7b. It pins nothing | implemented | | | C7d | C7b and C7c accept the frame the revision is stored on, a dataset frame of the same `spec_id` and rendering, or a derived frame whose recipe has the stored frame's `normal_form_hash`. Any other frame refuses `render_target_mismatch`, 409 | implemented | | -| C7e | On C7b and C7c every refusal answers the status the code registry maps for its code on `presentation.render`: `binding_invalid` and `constraint_violation` answer 422, and `frame_unavailable`, `release_mismatch`, and `resource_exhausted` answer 409. A component with no current revision refuses `edit_conflict`, 409. "Refusal code detail" below gives the statuses create and revise serve | implemented | | +| C7e | On C7b and C7c every refusal answers the status the code registry maps for its code on `presentation.render`: `binding_invalid` and `constraint_violation` answer 422, and `frame_unavailable`, `release_mismatch`, and `resource_exhausted` answer 409. A store that fails to answer refuses `service_unavailable`, 503. A component with no current revision refuses `edit_conflict`, 409. "Refusal code detail" below gives the statuses create and revise serve | implemented | | ## Request shape @@ -63,7 +63,7 @@ verified against `src/presentation_api` and the shared platform envelope it rend | C22 | When a shown value matches no frame row, two rows, a row with a non-null `status`, or a row whose `value` differs from the card's, and when a table revision's rows carry `series_id` on only some rows or carry a non-string or empty one, the route refuses `internal_error`, 500, logs the cause, and serves no file | implemented | | | C23 | Columns are `series` (the card's label for the row), then the frame's columns in frame order, with `stat_var_label`, `{role}_label`, and `provenance_label` after their columns, read from the frame's labels. A generated name that a frame column or an earlier generated column holds gains the first free suffix `_2`, `_3`, and so on, so every frame column keeps its own name. Files are written by the Datasets writers (`materialise.formats`), so a CSV or Excel cell starting with a formula trigger gains a leading apostrophe and Excel cells are literal strings | implemented | | | C24 | The bundle holds `README.md`, `data/{card-title-slug}.{format}`, `data/codes.csv` (when the rows hold codes), `SOURCES.md`, `CITATION.cff` and `datapackage.json` from `materialise.bundle.assemble_component_bundle`. Sources and licences come from the frame manifest's `citation`, one entry per provenance ref the rows carry, for `spec_` and `ops_` frames alike. A card export of a root `spec_` frame fills the citation fields the stored record left empty from a live read, and a failed read is not cached. The descriptor's `dfo` property holds `component` (`component_id`, `revision`, `frame_ref`, `select`, `window`), `prepared_at` (the revision's stored creation time, which also dates the file name), `links`, and `reproducing_call`, the export route as an absolute URL with its options and no header. The README takes the card title as its heading, says it holds the rows behind that revision, and names the frame's `spec_id` and `content_version`. The bytes match from one request to the next while the graph's code labels and the sources' documentation that a derived frame, or a root frame whose stored citation left fields empty, reads live are unchanged | implemented | | -| C25 | An unknown component or revision, a `note_narration` or `source_card`, a revision stored without `series_coordinates`, and a table revision none of whose rows carries `series_id` refuse `not_found`, 404, the last with a detail saying the revision predates table row identities and the card must be recreated to export it. A frame the store cannot load refuses `frame_unavailable` by its reason table. An unknown `format` or unparseable `bundle` refuses `request_invalid`, 400. The route shares the components door's semaphore and timeout, so saturation refuses `service_unavailable` and a slow export `timeout` | implemented | | +| C25 | An unknown component or revision, a `note_narration` or `source_card`, a revision stored without `series_coordinates`, and a table revision none of whose rows carries `series_id` refuse `not_found`, 404, the last with a detail saying the revision predates table row identities and the card must be recreated to export it. A frame the store cannot load refuses the code its reason names in "FrameUnavailable outcome by reason": `frame_unavailable`, 404, for a frame state, and `service_unavailable`, 503, for a store that fails to answer. An unknown `format` or unparseable `bundle` refuses `request_invalid`, 400. The route shares the components door's semaphore and timeout, so saturation refuses `service_unavailable` and a slow export `timeout` | implemented | | ## Span periods @@ -99,7 +99,7 @@ alone. | write_refused | 403 | do_not_retry | implemented | | | request_invalid | 400 | headline correct_and_retry; wire `retryable` always false | implemented | | | resource_exhausted | 400 | do_not_retry | implemented | | -| frame_unavailable | 404 | refresh_then_retry (expired, ops_unsupported) / do_not_retry (revoked, missing_rendering, digest_mismatch, quarantined, unknown) | implemented | | +| frame_unavailable | 404 | refresh_then_retry (expired, ops_unsupported, recipe_pending) / do_not_retry (revoked, missing_rendering, digest_mismatch, quarantined, unknown) | implemented | | | release_mismatch | 503 | retry_after_delay | implemented | | | service_unavailable | 503 | retry_after_delay | implemented | | | timeout | 504 | retry_after_delay | implemented | | @@ -110,7 +110,7 @@ alone. ### FrameUnavailable outcome by reason -`_FRAME_UNAVAILABLE_OUTCOME` (`presentation_api.refusals`) reads each status from the code registry on `presentation.write`. A `frame_unavailable` problem body's `cause.code` carries the reason. A reason outside this table fails classification, and the platform default answers `internal_error`. +`_FRAME_UNAVAILABLE_OUTCOME` (`presentation_api.refusals`) names the code and retryability of each reason in `assembler.ports.FrameUnavailableReason`, and importing the module fails if the two differ. HTTP is the status the code registry gives the code on the bound endpoint: this table shows the write, read and share routes, and on a render route `frame_unavailable` answers 409. `cause.code` carries the reason on every code. `recipe_pending` and `recipe_unreadable` refuse a pin, so only the share routes raise them. | Reason | Code | HTTP | retryable | | ----------------- | ------------------- | ---- | --------- | @@ -120,32 +120,35 @@ alone. | missing_rendering | frame_unavailable | 404 | false | | digest_mismatch | frame_unavailable | 404 | false | | quarantined | frame_unavailable | 404 | false | -| io | service_unavailable | 503 | true | -| recipe_pending | service_unavailable | 503 | true | -| recipe_unreadable | service_unavailable | 503 | true | +| recipe_pending | frame_unavailable | 404 | true | | unknown | frame_unavailable | 404 | false | +| io | service_unavailable | 503 | true | +| pending | service_unavailable | 503 | true | +| recipe_unreadable | internal_error | 500 | false | ## Registry binding -`presentation_api.codes.PRESENTATION_CODE_CATALOGUE` union -`presentation_api.codes.PRESENTATION_SERVED_PLATFORM_CODES`, the nine codes this surface mints -itself plus the seven platform codes its handlers, its `validation_problem` path, the -live-refresh check, or the platform default put on the wire (`not_found`, `request_invalid`, -`service_unavailable`, `timeout`, `internal_error`, `unsupported_schema_version`, -`refresh_not_permitted`), is exactly the set of active spec-kit registry entries whose mappings -name a `presentation.*` endpoint. `tests/platform/test_registry_binding.py`'s -`test_presentation_served_codes_match_registry` asserts this bijection and passes. +`presentation_api.codes.PRESENTATION_SERVED_CODES`, the 23 codes of +`PRESENTATION_CODE_CATALOGUE`, `SHARE_CODE_CATALOGUE`, `PRESENTATION_SERVED_PLATFORM_CODES` and +`WRITE_SERVED_PLATFORM_CODES`, is exactly the set of active spec-kit registry entries whose +mappings name a `presentation.*` endpoint. `tests/platform/test_registry_binding.py`'s +`test_served_codes_match_registry` asserts this bijection and passes. `PRESENTATION_SERVED_PLATFORM_CODES` is what carries `C12`'s `unsupported_schema_version` into that comparison: it has no `CODE_RETRY_MODE` entry, since that table mirrors the baked catalogue manifest's `retry_modes` byte for byte, so `CODE_RETRY_MODE`'s own key set alone would leave the code out of the served set on both sides. -The surface binds `presentation.write` to create and revise, `presentation.render` to C7b and -C7c, and `presentation.shares` to every shares door route (`Surface.registry_endpoints`). The -component read shares revise's path, so it is bound with it. The frames, component data, release, -and meta routes map no registry endpoint and serve the write routes' statuses. +Each route binds the registry endpoint it serves, keyed by method and path +(`Surface.registry_endpoints`): `presentation.write` to create and revise, +`presentation.component_read` to C5, `presentation.component_data` to the data route, +`presentation.render` to C7b and C7c, `presentation.frames` to both frames routes, and +`presentation.shares` to every shares door route. The release and meta routes serve no endpoint. +Every classifier reads a refusal's status from the registry under the bound endpoint +(`presentation_api.codes.served_status`), so no Presentation source holds one, and +`compute_presentation_capability_version` hashes the status of each code on each +`presentation.*` endpoint. A code a route raises that its endpoint leaves unmapped answers its +status on `presentation.write`, or else on the first endpoint that maps it, and logs an error. `tests/presentation_api/test_refusal_registry.py` enumerates the composed surface's routes and -asserts that every route but those is bound, that every `presentation.*` endpoint is bound, and -that every exemplar goes out at the status the registry gives its code: a render door exemplar on -`presentation.render`, a shares door exemplar on `presentation.shares`, and every other on -`presentation.write`. +asserts that every route but those is bound, that every `presentation.*` endpoint is bound, that +one door owns each endpoint, and that every exemplar and `FrameUnavailable` reason a route raises +goes out at the one status the registry gives its code there. diff --git a/packages/dfo-analysis-engine/pyproject.toml b/packages/dfo-analysis-engine/pyproject.toml index 433db76e..5f38b2d9 100644 --- a/packages/dfo-analysis-engine/pyproject.toml +++ b/packages/dfo-analysis-engine/pyproject.toml @@ -20,8 +20,8 @@ dependencies = [ # See the root pyproject.toml's dfo-spec-kit source comment for the # pin-consistency invariant these two revs must satisfy: every spec-kit source in the # workspace moves together as one rev. -dfo-spec-kit = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "2dd2400a36d592bd3ca814aa2eff30bd016433e3", subdirectory = "python/spec-kit" } -dfo-spec-kit-frame-grammar = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "2dd2400a36d592bd3ca814aa2eff30bd016433e3", subdirectory = "python/frame-grammar" } +dfo-spec-kit = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "32c47517128d6c4311c09bd5072203edd859fc48", subdirectory = "python/spec-kit" } +dfo-spec-kit-frame-grammar = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "32c47517128d6c4311c09bd5072203edd859fc48", subdirectory = "python/frame-grammar" } [build-system] requires = ["uv_build>=0.11,<0.12"] diff --git a/packages/dfo-data-tools/pyproject.toml b/packages/dfo-data-tools/pyproject.toml index 860969e3..ec83e3d1 100644 --- a/packages/dfo-data-tools/pyproject.toml +++ b/packages/dfo-data-tools/pyproject.toml @@ -88,10 +88,10 @@ graph-release = ["google-cloud-spanner>=3.57", "dfo-data-tools[coverage]"] [tool.uv.sources] # See the root pyproject.toml's dfo-spec-kit source comment for the # pin-consistency invariant this rev must satisfy. -dfo-spec-kit-frame-grammar = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "2dd2400a36d592bd3ca814aa2eff30bd016433e3", subdirectory = "python/frame-grammar" } -dfo-spec-kit-components = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "2dd2400a36d592bd3ca814aa2eff30bd016433e3", subdirectory = "python/dfo-spec-kit-components" } -dfo-spec-kit-provenance-docs = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "2dd2400a36d592bd3ca814aa2eff30bd016433e3", subdirectory = "python/provenance-docs" } -dfo-spec-kit-descriptions = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "2dd2400a36d592bd3ca814aa2eff30bd016433e3", subdirectory = "python/descriptions" } +dfo-spec-kit-frame-grammar = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "32c47517128d6c4311c09bd5072203edd859fc48", subdirectory = "python/frame-grammar" } +dfo-spec-kit-components = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "32c47517128d6c4311c09bd5072203edd859fc48", subdirectory = "python/dfo-spec-kit-components" } +dfo-spec-kit-provenance-docs = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "32c47517128d6c4311c09bd5072203edd859fc48", subdirectory = "python/provenance-docs" } +dfo-spec-kit-descriptions = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "32c47517128d6c4311c09bd5072203edd859fc48", subdirectory = "python/descriptions" } [build-system] requires = ["uv_build>=0.11,<0.12"] diff --git a/packages/dfo-data-tools/src/assembler/ports.py b/packages/dfo-data-tools/src/assembler/ports.py index 34482c2b..8bff6134 100644 --- a/packages/dfo-data-tools/src/assembler/ports.py +++ b/packages/dfo-data-tools/src/assembler/ports.py @@ -176,15 +176,28 @@ class ResolvedFrame: rendering: Mapping[str, object] | None = None +# Why a frame does not load. Publishing an `ops_`/`ocv_` pair can make `ops_unsupported` succeed +# on retry. `recipe_pending` (a derived frame with no recipe yet), which a recipe write clears, and +# `recipe_unreadable` (a store with no recipe reader, or a recipe root naming no dataset version), +# which no retry clears, refuse a pin. `pending` is a newest version not ready to serve, which a +# retry may find ready. `io` is a store that fails to answer. +FrameUnavailableReason = Literal[ + "revoked", + "expired", + "missing_rendering", + "digest_mismatch", + "ops_unsupported", + "quarantined", + "recipe_pending", + "recipe_unreadable", + "pending", + "io", + "unknown", +] + + class FrameUnavailable(Exception): - # reason: "revoked" | "expired" | "missing_rendering" | "digest_mismatch" | - # "ops_unsupported" | "quarantined" | "recipe_pending" | "recipe_unreadable" | "io" | - # "unknown" - # Publishing an `ops_`/`ocv_` pair can make `ops_unsupported` succeed on retry. - # `recipe_pending` (a derived frame with no recipe yet) and `recipe_unreadable` (a store with - # no recipe reader, or a recipe root naming no dataset version) refuse a pin, and both are - # retryable. - def __init__(self, *, reason: str, detail: str) -> None: + def __init__(self, *, reason: FrameUnavailableReason, detail: str) -> None: self.reason = reason self.detail = detail super().__init__(detail) diff --git a/packages/dfo-data-tools/src/dfo_platform/app.py b/packages/dfo-data-tools/src/dfo_platform/app.py index 40c24b1a..31fc2d36 100644 --- a/packages/dfo-data-tools/src/dfo_platform/app.py +++ b/packages/dfo-data-tools/src/dfo_platform/app.py @@ -45,6 +45,7 @@ from dfo_platform.compression import BufferedGzipMiddleware from dfo_platform.config import executor_max_workers as _default_executor_max_workers from dfo_platform.cors import PlatformCORSMiddleware +from dfo_platform.doors import RouteKey, route_keys from dfo_platform.executor import SaturationTrackingExecutor, current_thread_is_owned_by from dfo_platform.problem import ( PROBLEM_JSON, @@ -100,8 +101,9 @@ class Surface: before `call_next`. Returning a spec as data avoids the `ServerErrorMiddleware` re-raise caused by exceptions in this layer. - `registry_endpoints` maps a route's path, as the router declares it, to the - code registry endpoint it serves, such as `datasets.mint`. Admission checks + `registry_endpoints` maps a route's `doors.RouteKey`, its method and its path + as the router declares it, to the code registry endpoint it serves, such as + `datasets.mint`, so two methods on one path can serve two endpoints. Admission checks and exception classification for a request that route matches run with that endpoint bound (`codes.bound_registry_endpoint`), so `registered_status` reads the route's own status for a code. @@ -137,7 +139,7 @@ class Surface: exception_exemplars: tuple[BaseException, ...] = () lifespan: Callable[[FastAPI], AbstractAsyncContextManager[None]] | None = None middleware: tuple[Callable[[Request], ProblemSpec | None], ...] = () - registry_endpoints: Mapping[str, str] = field(default_factory=dict) + registry_endpoints: Mapping[RouteKey, str] = field(default_factory=dict) request_scope: Callable[[], AbstractContextManager[object]] | None = None public_paths: frozenset[str] = frozenset() description: str = "" @@ -269,8 +271,7 @@ def _validate_surfaces(surfaces: Sequence[Surface]) -> None: "another surface's handler or the platform default instead of this " "surface's own." ) - route_paths = {getattr(route, "path", None) for route in surface.router.routes} - unrouted = sorted(set(surface.registry_endpoints) - route_paths) + unrouted = sorted(set(surface.registry_endpoints) - route_keys(surface.router)) if unrouted: raise SurfaceConfigurationError( f"surface {surface.name!r} names registry endpoints for {unrouted!r}, which " @@ -814,7 +815,8 @@ def _registry_endpoint(surface: Surface, scope: Scope) -> str | None: the router would pick it, or `None` when that route names none.""" for route in surface.router.routes: if isinstance(route, BaseRoute) and route.matches(scope)[0] == Match.FULL: - return surface.registry_endpoints.get(getattr(route, "path", "")) + key = (scope.get("method", ""), getattr(route, "path", "")) + return surface.registry_endpoints.get(key) return None diff --git a/packages/dfo-data-tools/src/dfo_platform/doors.py b/packages/dfo-data-tools/src/dfo_platform/doors.py index 7f25aabb..59aebc5d 100644 --- a/packages/dfo-data-tools/src/dfo_platform/doors.py +++ b/packages/dfo-data-tools/src/dfo_platform/doors.py @@ -12,6 +12,9 @@ from dfo_platform import ProblemSpec from dfo_platform.codes import bound_endpoint +# A route as `registry_endpoints` names it: its method, and its path as the router declares it. +type RouteKey = tuple[str, str] + @dataclass(frozen=True) class DoorContribution: @@ -20,7 +23,7 @@ class DoorContribution: Routers carry full paths. Classifiers return `None` for unrecognised exceptions. Lifespans enter together. Middleware checks run synchronously before FastAPI parses the request body. Each check decides for itself, for example by path, whether a request concerns it. - `registry_endpoints` maps each route path to the code registry endpoint it serves + `registry_endpoints` maps each route's `RouteKey` to the code registry endpoint it serves (`dfo_platform.app.Surface.registry_endpoints`), and `public_paths` names the route paths any origin may read (`dfo_platform.app.Surface.public_paths`). """ @@ -31,7 +34,7 @@ class DoorContribution: exception_exemplars: tuple[BaseException, ...] = () lifespan: Callable[[FastAPI], AbstractAsyncContextManager[None]] | None = None middleware: tuple[Callable[[Request], ProblemSpec | None], ...] = () - registry_endpoints: Mapping[str, str] = field(default_factory=dict) + registry_endpoints: Mapping[RouteKey, str] = field(default_factory=dict) public_paths: frozenset[str] = frozenset() @@ -86,6 +89,16 @@ def flatten(doors: Iterable[DoorContribution]) -> APIRouter: return router -def route_registry_endpoints(router: APIRouter, endpoint: str) -> dict[str, str]: +def route_keys(router: APIRouter) -> frozenset[RouteKey]: + """The `RouteKey` of each method each route on `router` answers.""" + return frozenset( + (method, route.path) + for route in router.routes + if isinstance(route, APIRoute) + for method in route.methods or () + ) + + +def route_registry_endpoints(router: APIRouter, endpoint: str) -> dict[RouteKey, str]: """Map every route on `router` to the one code registry `endpoint` its door serves.""" - return {route.path: endpoint for route in router.routes if isinstance(route, APIRoute)} + return dict.fromkeys(route_keys(router), endpoint) diff --git a/packages/dfo-data-tools/src/dfo_platform/problem.py b/packages/dfo-data-tools/src/dfo_platform/problem.py index 204db88b..4f40c42b 100644 --- a/packages/dfo-data-tools/src/dfo_platform/problem.py +++ b/packages/dfo-data-tools/src/dfo_platform/problem.py @@ -35,9 +35,9 @@ class Alternative(BaseModel): class Cause(BaseModel): """The `cause` extension: why a refusal fired, beneath its own top-level `code` -- present only on a refusal that carries one (`recipe_not_declared`, - `rate_coverage_gap`, `operation_failed`, `frame_unavailable`). On - `operation_failed`, `code` is the failed operation's refusal code, and on - `frame_unavailable` the reason the frame does not load. Mirrors `alternatives`/`truncated`'s + `rate_coverage_gap`, `operation_failed`, or a Presentation refusal of a frame). On + `operation_failed`, `code` is the failed operation's refusal code, and on a + frame's refusal the reason the frame does not load. Mirrors `alternatives`/`truncated`'s own extension style: `entities` names what the cause concerns (entity or `entity@year` keys), capped, with `truncated` set when the cap cut items short. @@ -47,7 +47,7 @@ class Cause(BaseModel): description=( "the cause code: a sub-cause such as nominal_undeclared, recipe_undeclared, or " "rate_missing, on operation_failed the failed operation's own refusal code, or on " - "frame_unavailable the reason the frame does not load" + "a Presentation refusal of a frame the reason the frame does not load" ) ) entities: list[str] = Field( diff --git a/pyproject.toml b/pyproject.toml index 31638fda..8cc3bf07 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -251,7 +251,7 @@ dfo-analysis-engine = { workspace = true } # The registry refuses a part's `source_code` on search.resolve with # `source_code_not_held`, `source_code_ambiguous`, and # `source_code_shares_variable`. -# The pin is spec-kit 2dd2400, which includes dev's 5132aa0, the share import +# The pin is spec-kit 32c4751, which includes dev's 5132aa0, the share import # route's refusal codes. It adds ref format 2 (derived # members) with roots that may carry the groupby DFO read them with, `mv_` # minted variable names, the output-digest and analysis rule vectors, the @@ -292,10 +292,16 @@ dfo-analysis-engine = { workspace = true } # a rebuild of a bare `spec_id`'s newest version can raise, # registers `provenance_unavailable` and `licence_not_redistributable`, and adds the # mcp_refusal channel with the MCP tool refusals on mcp.tools. +# It adds presentation.frames, presentation.component_read and +# presentation.component_data for the Presentation read routes, maps +# `unsupported_schema_version` (422) on presentation.render and +# presentation.shares, maps `binding_invalid` (400) on +# presentation.component_data, and states how a pin refuses a derived frame's +# recipe. # Follow-up: move all eight to a real spec-kit tag once one is cut, so a future # force-push or GC on spec-kit's dev can't silently break this lock. -dfo-spec-kit = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "2dd2400a36d592bd3ca814aa2eff30bd016433e3", subdirectory = "python/spec-kit" } -dfo-spec-kit-client-contract = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "2dd2400a36d592bd3ca814aa2eff30bd016433e3", subdirectory = "python/client-contract" } +dfo-spec-kit = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "32c47517128d6c4311c09bd5072203edd859fc48", subdirectory = "python/spec-kit" } +dfo-spec-kit-client-contract = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git", rev = "32c47517128d6c4311c09bd5072203edd859fc48", subdirectory = "python/client-contract" } [tool.pytest.ini_options] testpaths = ["tests", "packages/dfo-analysis-engine/tests"] diff --git a/src/datasets_api/app.py b/src/datasets_api/app.py index 99ed2361..0b90c006 100644 --- a/src/datasets_api/app.py +++ b/src/datasets_api/app.py @@ -296,7 +296,7 @@ async def lifespan(_app: FastAPI): lifespan=lifespan, middleware=tuple(check for door in doors for check in door.middleware), registry_endpoints={ - path: endpoint for door in doors for path, endpoint in door.registry_endpoints.items() + key: endpoint for door in doors for key, endpoint in door.registry_endpoints.items() }, request_scope=request_scope, ) diff --git a/src/datasets_api/doors/definition.py b/src/datasets_api/doors/definition.py index 3d732df0..0fa90e72 100644 --- a/src/datasets_api/doors/definition.py +++ b/src/datasets_api/doors/definition.py @@ -1010,9 +1010,10 @@ async def post_definition_members( return DoorContribution( router=router, registry_endpoints={ - "/api/datasets/{id:dfo_spec_id}": "datasets.definition", - "/api/datasets/{id:dfo_spec_id}/options": "datasets.options", - "/api/datasets/{id:dfo_spec_id}/members": "datasets.members", + ("GET", "/api/datasets/{id:dfo_spec_id}"): "datasets.definition", + ("GET", "/api/datasets/{id:dfo_spec_id}/options"): "datasets.options", + ("POST", "/api/datasets/{id:dfo_spec_id}/options"): "datasets.options", + ("POST", "/api/datasets/{id:dfo_spec_id}/members"): "datasets.members", }, classify=classify_definition_exception, exception_types=tuple(dict.fromkeys(chain.from_iterable(ROUTE_EXCEPTION_TYPES.values()))), diff --git a/src/datasets_api/doors/derived.py b/src/datasets_api/doors/derived.py index 5b54ad9a..c2293e70 100644 --- a/src/datasets_api/doors/derived.py +++ b/src/datasets_api/doors/derived.py @@ -225,7 +225,7 @@ def build_derived_door(*, deps: DepsCell) -> DoorContribution: ) return DoorContribution( router=router, - registry_endpoints={MANIFEST_PATH: "datasets.snapshot"}, + registry_endpoints={("GET", MANIFEST_PATH): "datasets.snapshot"}, classify=classify_datasets_refusal, exception_types=( SnapshotNotFound, @@ -270,7 +270,7 @@ def build_derived_files_door(*, deps: DepsCell) -> DoorContribution: ) return DoorContribution( router=router, - registry_endpoints={FILE_PATH: "datasets.files"}, + registry_endpoints={("GET", FILE_PATH): "datasets.files"}, classify=classify_files_exception, exception_types=( SnapshotNotFound, diff --git a/src/datasets_api/doors/exports.py b/src/datasets_api/doors/exports.py index 09f02f04..a975eeb9 100644 --- a/src/datasets_api/doors/exports.py +++ b/src/datasets_api/doors/exports.py @@ -152,7 +152,7 @@ async def post_export(body: ExportRequestBody, request: Request) -> JSONResponse exemplar_day = datetime(2026, 1, 1, tzinfo=UTC) return DoorContribution( router=router, - registry_endpoints={PREPARE_PATH: "datasets.exports"}, + registry_endpoints={("POST", PREPARE_PATH): "datasets.exports"}, classify=classify_files_exception, exception_types=( SpecNotFound, @@ -248,7 +248,7 @@ async def get_export(export_id: str) -> RedirectResponse: return DoorContribution( router=router, - registry_endpoints={LINK_PATH: "datasets.export_link"}, + registry_endpoints={("GET", LINK_PATH): "datasets.export_link"}, classify=classify_files_exception, exception_types=( ExportNotFound, diff --git a/src/datasets_api/doors/meta.py b/src/datasets_api/doors/meta.py index e76f420b..e5d40d53 100644 --- a/src/datasets_api/doors/meta.py +++ b/src/datasets_api/doors/meta.py @@ -491,5 +491,7 @@ async def get_operation(operation_id: str) -> JSONResponse: OperationBackpressure(), OperationBacklogFull(in_flight=1, limit=1), ), - registry_endpoints={"/api/datasets/operations/{operation_id}": "datasets.operations"}, + registry_endpoints={ + ("GET", "/api/datasets/operations/{operation_id}"): "datasets.operations" + }, ) diff --git a/src/graph_api/app.py b/src/graph_api/app.py index 3bf0170d..d310857a 100644 --- a/src/graph_api/app.py +++ b/src/graph_api/app.py @@ -415,7 +415,7 @@ def request_scope() -> Iterator[None]: ), lifespan=lifespan, registry_endpoints={ - path: endpoint for door in doors for path, endpoint in door.registry_endpoints.items() + key: endpoint for door in doors for key, endpoint in door.registry_endpoints.items() }, request_scope=request_scope, ) diff --git a/src/graph_api/doors/members.py b/src/graph_api/doors/members.py index 150cd705..b94db4af 100644 --- a/src/graph_api/doors/members.py +++ b/src/graph_api/doors/members.py @@ -740,8 +740,8 @@ def build_members_door() -> DoorContribution: return DoorContribution( router=router, registry_endpoints={ - "/api/graph/node/{dcid:dcid}/members": "graph.members", - "/api/graph/roots": "graph.roots", + ("GET", "/api/graph/node/{dcid:dcid}/members"): "graph.members", + ("GET", "/api/graph/roots"): "graph.roots", }, classify=classify_members_exception, exception_types=( diff --git a/src/graph_api/doors/node.py b/src/graph_api/doors/node.py index 488c4259..dddc3794 100644 --- a/src/graph_api/doors/node.py +++ b/src/graph_api/doors/node.py @@ -818,8 +818,8 @@ def build_node_door() -> DoorContribution: return DoorContribution( router=router, registry_endpoints={ - "/api/graph/node/{dcid:dcid}": "graph.node", - "/api/graph/nodes": "graph.nodes", + ("GET", "/api/graph/node/{dcid:dcid}"): "graph.node", + ("POST", "/api/graph/nodes"): "graph.nodes", }, classify=classify_node_exception, exception_types=( diff --git a/src/graph_api/doors/sources.py b/src/graph_api/doors/sources.py index 2ed6d9cc..d7ec5e76 100644 --- a/src/graph_api/doors/sources.py +++ b/src/graph_api/doors/sources.py @@ -444,7 +444,7 @@ def build_sources_door() -> DoorContribution: ) return DoorContribution( router=router, - registry_endpoints={"/api/graph/documentation/{dcid:dcid}": "graph.documentation"}, + registry_endpoints={("GET", "/api/graph/documentation/{dcid:dcid}"): "graph.documentation"}, classify=classify_sources_exception, exception_types=( SourceDocumentNotFound, diff --git a/src/presentation_api/README.md b/src/presentation_api/README.md index 2fae7549..27968ad9 100644 --- a/src/presentation_api/README.md +++ b/src/presentation_api/README.md @@ -55,7 +55,7 @@ Query parameters: - `dimension`: Isolates a single dimension and returns its complete list of permissible values. - `query`: Filters dimension values across all facets by substring match. -`GET /api/presentation/frames/{spec_id}` names a dataset by its `spec_` id alone (`spec_` and 32 lowercase hex digits; any other id refuses 400 `request_invalid`). It reads the version the analysis runner reads for a Spec root `{spec_id}` (`datasets_api.analysis_roots.LiveRootSource.resolve`): the served definition's newest version, rebuilt when its head is stale. It takes the same `dimension` and `query` and answers the pair route's body, read from that version's canonical frame, so it holds no default enrichment column and a `dimension` such as `financingProvider_type` refuses 422 `binding_invalid`. It adds two fields, `spec_id`, the served id it read (another id when the one sent serves under it), and `content_version`. A version still building refuses 503 `service_unavailable`, retryable. Any other Datasets refusal refuses 404 `frame_unavailable`, not retryable, with the Datasets code in its `detail`, and `cause.code` `revoked` for a revoked version, `quarantined` for a quarantined identity and `unknown` for the rest. An app that does not mount the Datasets surface resolves no bare id and refuses 404 `frame_unavailable`. +`GET /api/presentation/frames/{spec_id}` names a dataset by its `spec_` id alone (`spec_` and 32 lowercase hex digits; any other id refuses 400 `request_invalid`). It reads the version the analysis runner reads for a Spec root `{spec_id}` (`datasets_api.analysis_roots.LiveRootSource.resolve`): the served definition's newest version, rebuilt when its head is stale. It takes the same `dimension` and `query` and answers the pair route's body, read from that version's canonical frame, so it holds no default enrichment column and a `dimension` such as `financingProvider_type` refuses 400 `binding_invalid`. It adds two fields, `spec_id`, the served id it read (another id when the one sent serves under it), and `content_version`. A version still building, or a Datasets refusal a retry may clear, refuses 503 `service_unavailable`, retryable, with `cause.code` `pending`. Any other Datasets refusal refuses 404 `frame_unavailable`, not retryable, with the Datasets code in its `detail`, and `cause.code` `revoked` for a revoked version, `quarantined` for a quarantined identity and `unknown` for the rest. An app that does not mount the Datasets surface resolves no bare id and refuses 404 `frame_unavailable`. Each dimension lists its non-null values in `values`, counted by `total_count`. `plural` says whether the frame's series hold more than one value of the dimension, a null counting as one, so a `unit` dimension holding `USDollar` on some series and null on others is plural with one listed value. A series its source's estimates continue holds the method of the rows it continues (`assembler.frame_view.series_keys`), so on a frame whose only other method is that continuation `measurementMethod` is not plural, and its continuing method stays listed as a value a select binds to draw the estimates alone. @@ -117,9 +117,9 @@ The create returns a `shw_` write token once, and the store keeps its SHA-256 di | `not_found` | 404 | No share at the id, or it was taken down; on a create or replace, a card or revision the store lacks; on a data read, a slot the share has not had, one a replacement left empty, or a note's. | | `request_invalid` | 400 | A malformed body (`latest` missing or not a boolean, a card count or `latest` the `kind` does not take, more than 24 cards, an unknown field), id, slot or format, or a website request whose `x-dfo-caller` is missing or malformed. | | `payload_too_large` | 413 | A create's `payload` is over 10 MiB as compact UTF-8 (`presentation_store.MAX_SHARE_PAYLOAD_BYTES`). | -| `frame_unavailable` | 404 | A card's frame is gone: unknown, expired, revoked or quarantined, as on the component routes. | -| `service_unavailable` | 503 | A frame read or store call failed, or the door is at capacity. Retryable. A pin of a derived frame with no recipe yet answers it too (`recipe_pending`) and succeeds once its recipe is written. One with no recipe that a reference already names pins alone (`presentation_store` README, pin step 3). A pin whose recipe cannot be read (`recipe_unreadable`) answers it as well, for a store with no recipe reader or a recipe root naming no dataset version. | -| `internal_error` | 500 | The caller cannot be named: `DFO_CALLER_HMAC_KEY` is unset or the trusted `X-Forwarded-For` entry is missing. | +| `frame_unavailable` | 404 | A card's frame is gone: unknown, expired, revoked or quarantined, as on the component routes. A pin of a derived frame with no recipe yet answers it too, retryable, with `cause.code` `recipe_pending`, and succeeds once its recipe is written. One with no recipe that a reference already names pins alone (`presentation_store` README, pin step 3). | +| `service_unavailable` | 503 | A frame read or store call failed, or the door is at capacity. Retryable. | +| `internal_error` | 500 | The caller cannot be named: `DFO_CALLER_HMAC_KEY` is unset or the trusted `X-Forwarded-For` entry is missing. A pin whose recipe cannot be read answers it too, with `cause.code` `recipe_unreadable`, for a store with no recipe reader or a recipe root naming no dataset version. Recipes are write-once, so no retry clears it. | The shares door mounts when the surface builds its stores live or is given a `share_store`. A surface composed over an injected `revision_store` without one serves no share route. @@ -503,7 +503,7 @@ The per-response `retryable` field is authoritative for every code. `retry_modes | `binding_invalid` | 400 | `correct_and_retry` | Unbound selectors or invalid values. Returns `alternatives`, `truncated`, and `issues`. Each alternative pairs a legal value with the label the frame resolved for it, falling back to the value itself when the frame names none. | | `constraint_violation` | 400 | `correct_and_retry` | Violates catalogue constraints. Returns `constraint_id`, `pointer`, and `detail`. `pointer` is expressed against the intent, for example `/scale`, `/range`, or `/change/basis`. | | `edit_conflict` | 409 | `refresh_then_retry` | The component, a requested revision, or on create the referenced composition vanished or was rewritten mid flight, raised by a scoped or unscoped `GET` as well as by writes. | -| `frame_unavailable` | 404 | Contextual | `cause.code` carries the reason named here, so a client tells a frame it can rebuild from one taken down. `unknown` (404, not retryable) names a `spec_id`/`content_version` pair that was never stored. `expired` (404, retryable) names a stored version that aged out, and its `detail` ends by naming a fetch of the ref with latest=True to draw from current data. `ops_unsupported` (404, retryable) names an `ops_`/`ocv_` pair analysis-mcp minted but never published to the platform; publishing it is the retry that succeeds. `revoked` and the other permanent reasons (`missing_rendering`, `digest_mismatch`) return 404, not retryable. `quarantined` (404, not retryable) names a `spec_id` the spec registry holds in quarantine. A frame store that fails to answer (`io`) and a pin refused `recipe_pending` or `recipe_unreadable` answer `service_unavailable` instead. | +| `frame_unavailable` | 404 | Contextual | `cause.code` carries the reason named here, so a client tells a frame it can rebuild from one taken down. `unknown` (404, not retryable) names a `spec_id`/`content_version` pair that was never stored. `expired` (404, retryable) names a stored version that aged out, and its `detail` ends by naming a fetch of the ref with latest=True to draw from current data. `ops_unsupported` (404, retryable) names an `ops_`/`ocv_` pair analysis-mcp minted but never published to the platform; publishing it is the retry that succeeds. `revoked` and the other permanent reasons (`missing_rendering`, `digest_mismatch`) return 404, not retryable. `quarantined` (404, not retryable) names a `spec_id` the spec registry holds in quarantine. A frame store that fails to answer (`io`) refuses `service_unavailable` instead. | | `resource_exhausted` | 400 | `do_not_retry` | Selection exceeds allowed limits, including a minted component whose `component`, `provenance` and `description` subtrees serialise past `MAX_ENVELOPE_BYTES` (2 MiB, compact UTF-8 JSON), refused before any store write. Returns `limit_name` in `detail`. | | `release_mismatch` | 503 | `retry_after_delay` | The component's head carries a newer catalogue release than this deployment writes, which happens after a rollback and clears once the newer release is deployed again, or a stored payload has a shape the release's schema does not recognise, such as a bar_chart row migration 0006 has not rewritten. | | `request_invalid` | 400 | `correct_and_retry` | Retryable is always `false`. A malformed body field, an unparseable path identifier, or a non-positive `revision` query parameter. Also a create `write_token` reused with a different request. | @@ -513,7 +513,7 @@ The per-response `retryable` field is authoritative for every code. `retry_modes | `not_found` | 404 | `do_not_retry` | Unknown component or composition id (`DraftNotFound`), or a revision number never minted for a revision-scoped `GET`. | | `write_refused` | 403 | `do_not_retry` | A revise without the component's write token, or a create into an existing composition without that composition's token, in `x-dfo-write-token`. Also a draft created before write tokens. `detail` ends with a fix: create a new component. | | `allowance_exhausted` | 429 | `do_not_retry` | A create or revise past the caller's daily card allowance (`DATASETS_CARD_DAILY_ALLOWANCE`). `Retry-After` names the seconds to the next UTC midnight. A platform code outside the release document's `retry_modes`. A caller holding the `publish` grant, and a create retried with its first `write_token`, spend none. | -| `service_unavailable` | 503 | `retry_after_delay` | Database failure, a frame store that fails to answer, a pin of a derived frame whose recipe is unwritten or unreadable, or door thread pool backpressure exhaustion. | +| `service_unavailable` | 503 | `retry_after_delay` | Database failure, a frame store that fails to answer, or door thread pool backpressure exhaustion. | | `timeout` | 504 | `retry_after_delay` | Execution exceeded door timeout threshold. | | `internal_error` | 500 | `do_not_retry` | Permanent non-retryable store or system failure. | diff --git a/src/presentation_api/app.py b/src/presentation_api/app.py index e8b26ffe..3cd9619a 100644 --- a/src/presentation_api/app.py +++ b/src/presentation_api/app.py @@ -194,7 +194,7 @@ def request_scope() -> Iterator[None]: ), lifespan=lifespan, registry_endpoints={ - path: endpoint for door in doors for path, endpoint in door.registry_endpoints.items() + key: endpoint for door in doors for key, endpoint in door.registry_endpoints.items() }, request_scope=request_scope, public_paths=frozenset(path for door in doors for path in door.public_paths), diff --git a/src/presentation_api/codes.py b/src/presentation_api/codes.py index bf4482e5..642ff8f1 100644 --- a/src/presentation_api/codes.py +++ b/src/presentation_api/codes.py @@ -9,18 +9,16 @@ `schema_invalid` and `frame_unavailable` vary by cause. Each response's `retryable` value is authoritative. `request_invalid` has a retryable headline mode but always returns false. -`schema_invalid`, `binding_invalid`, `constraint_violation`, `request_invalid`, and -`resource_exhausted` return 400. `write_refused` returns 403, `edit_conflict` returns 409, and -`not_found` returns 404. The render routes refuse a caller other than the live-refresh -principal with `refresh_not_permitted` (403) and a target frame that may not stand in for the -stored one with `render_target_mismatch` (409). They serve `binding_invalid` and -`constraint_violation` at 422 and `frame_unavailable`, `release_mismatch`, and -`resource_exhausted` at 409 (`presentation_api.doors.render`). Their caller replays a stored -intent and cannot correct it, so there `retryable` is true only when the same request may -succeed later: `replay_retryable` keeps an occurrence's value for a `retry_after_delay` or -`refresh_then_retry` code and answers false for every other mode. -Frame unavailability returns 404, and a frame store that fails to answer returns -`service_unavailable`. Release mismatch and service unavailability return 503. Timeout returns 504, and internal errors return 500. +Every classifier reads a refusal's status from the code registry under the endpoint the platform +binds for the route (`served_status`), and `presentation_statuses` holds the registry's map for +the surface. `schema_invalid`, `binding_invalid`, `constraint_violation`, `request_invalid`, and +`resource_exhausted` are 400 on the write routes, and `frame_unavailable` 404. The render routes +serve `binding_invalid` and `constraint_violation` at 422 and `frame_unavailable`, +`release_mismatch`, and `resource_exhausted` at 409. Their caller replays a stored intent and +cannot correct it, so there `retryable` is true only when the same request may succeed later: +`replay_retryable` keeps an occurrence's value for a `retry_after_delay` or `refresh_then_retry` +code and answers false for every other mode. `presentation_api.refusals` names the code of each +`FrameUnavailable` reason. Missing-field `schema_invalid`, `binding_invalid`, and `constraint_violation` use `correct_and_retry`, which `retryable_for` answers false. The request must change before it can @@ -34,7 +32,7 @@ `write_token_required`, `write_token_invalid` and `share_import_not_permitted` at 403, `share_not_replaceable` and `share_id_taken` at 409 and `payload_too_large` at 413, none retryable. A card create or revise and a share create refuse -the platform's `allowance_exhausted` at 429, not retryable either, with `Retry-After` naming +the platform's `allowance_exhausted`, not retryable either, with `Retry-After` naming the reset (`WRITE_SERVED_PLATFORM_CODES`). The component catalogue's release manifest lists none of these, so they stay out of `CODE_RETRY_MODE`. `PRESENTATION_SERVED_CODES` is every code the surface serves. @@ -46,20 +44,50 @@ from __future__ import annotations -from dfo_platform.codes import PLATFORM_CODE_CATALOGUE, CodeCatalogue, merge_catalogues +import logging +from collections.abc import Mapping +from functools import cache +from types import MappingProxyType + +from dfo_spec_kit.registry import REGISTRY + +from dfo_platform.codes import ( + PLATFORM_CODE_CATALOGUE, + CodeCatalogue, + merge_catalogues, + registered_status, +) __all__ = [ "CODE_CATALOG", "CODE_RETRY_MODE", + "COMPONENT_DATA_ENDPOINT", + "COMPONENT_READ_ENDPOINT", + "FRAMES_ENDPOINT", "PRESENTATION_CODE_CATALOGUE", "PRESENTATION_SERVED_CODES", "PRESENTATION_SERVED_PLATFORM_CODES", + "RENDER_ENDPOINT", + "SHARES_ENDPOINT", "SHARE_CODE_CATALOGUE", + "WRITE_ENDPOINT", "WRITE_SERVED_PLATFORM_CODES", + "presentation_statuses", "replay_retryable", "retryable_for", + "served_status", ] +logger = logging.getLogger(__name__) + +# The code registry endpoints the surface's routes serve. +WRITE_ENDPOINT = "presentation.write" +COMPONENT_READ_ENDPOINT = "presentation.component_read" +COMPONENT_DATA_ENDPOINT = "presentation.component_data" +RENDER_ENDPOINT = "presentation.render" +FRAMES_ENDPOINT = "presentation.frames" +SHARES_ENDPOINT = "presentation.shares" + PRESENTATION_CODE_CATALOGUE: dict[str, tuple[str, str]] = { "schema_invalid": ( "Schema Invalid", @@ -83,7 +111,7 @@ ), "frame_unavailable": ( "Frame Unavailable", - "The referenced frame cannot be loaded right now.", + "The referenced frame is not held in a state Presentation can load.", ), "resource_exhausted": ( "Resource Exhausted", @@ -224,6 +252,42 @@ def replay_retryable(code: str, *, retryable: bool) -> bool: return retryable and CODE_RETRY_MODE.get(code) in _UNCHANGED_RETRY_MODES +@cache +def presentation_statuses() -> Mapping[str, Mapping[str, int]]: + """The status the code registry gives each code's problem details, by `presentation.*` + endpoint.""" + statuses: dict[str, dict[str, int]] = {} + for entry in REGISTRY.entries: + for mapping in entry.mappings: + if ( + mapping.endpoint.startswith("presentation.") + and mapping.channel == "problem_details" + and mapping.http_status is not None + ): + statuses.setdefault(mapping.endpoint, {})[entry.code] = mapping.http_status + return MappingProxyType({endpoint: statuses[endpoint] for endpoint in sorted(statuses)}) + + +def served_status(code: str) -> int: + """The status the code registry gives `code` on the bound endpoint. A code that endpoint + does not map answers the status of the first Presentation endpoint that does, + `presentation.write` before the others, and logs an error, so the client reads the refusal + and not a 500. `tests/presentation_api/test_refusal_registry.py` fails on such a pair. + + Raises: + LookupError: No Presentation endpoint maps `code`. + """ + try: + return registered_status(code) + except LookupError: + statuses = presentation_statuses() + for endpoint in (WRITE_ENDPOINT, *statuses): + if code in statuses[endpoint]: + logger.error("The bound registry endpoint maps no status for %s", code) + return statuses[endpoint][code] + raise + + def _catalogue_covers(catalogue: CodeCatalogue) -> None: """Raise at import if a retry-mode code is absent from the merged catalogue.""" missing = set(CODE_RETRY_MODE) - set(catalogue) diff --git a/src/presentation_api/doors/component_data.py b/src/presentation_api/doors/component_data.py index b5a11936..2d0a4ec2 100644 --- a/src/presentation_api/doors/component_data.py +++ b/src/presentation_api/doors/component_data.py @@ -28,7 +28,7 @@ from datasets_api.artifact_delivery import artifact_content_disposition from dfo_platform import ProblemSpec from materialise.formats import ExcelIllegalCharacterError -from presentation_api.codes import retryable_for +from presentation_api.codes import retryable_for, served_status from presentation_api.path_params import require_component_id if TYPE_CHECKING: @@ -49,7 +49,7 @@ def classify_component_data_exception(exc: BaseException) -> ProblemSpec | None: if isinstance(exc, ComponentHoldsNoData): return ProblemSpec( - status=404, + status=served_status("not_found"), code="not_found", detail=exc.detail, retryable=retryable_for("not_found"), @@ -57,7 +57,7 @@ def classify_component_data_exception(exc: BaseException) -> ProblemSpec | None: if isinstance(exc, ComponentExportMismatch): logger.error("Component data export refused: %s", exc) return ProblemSpec( - status=500, + status=served_status("internal_error"), code="internal_error", detail="the frame rows behind this revision do not match the values it shows", retryable=retryable_for("internal_error"), @@ -65,7 +65,7 @@ def classify_component_data_exception(exc: BaseException) -> ProblemSpec | None: if isinstance(exc, ExcelIllegalCharacterError): # The writer's detail tells the caller to request csv or parquet instead. return ProblemSpec( - status=400, + status=served_status("request_invalid"), code="request_invalid", detail=exc.detail or str(exc), retryable=False, diff --git a/src/presentation_api/doors/components.py b/src/presentation_api/doors/components.py index 78b3073a..af9d98cb 100644 --- a/src/presentation_api/doors/components.py +++ b/src/presentation_api/doors/components.py @@ -99,7 +99,14 @@ from dfo_platform.config import CallerSettings, PublicSiteSettings from dfo_platform.doors import DoorContribution, combine_classifiers from dfo_platform.store_errors import store_failure_is_permanent -from presentation_api.codes import retryable_for +from presentation_api.codes import ( + COMPONENT_DATA_ENDPOINT, + COMPONENT_READ_ENDPOINT, + RENDER_ENDPOINT, + WRITE_ENDPOINT, + retryable_for, + served_status, +) from presentation_api.config import ( PRESENTATION_COMPONENTS_MAX_CONCURRENCY, PRESENTATION_COMPONENTS_TIMEOUT_SECONDS, @@ -108,27 +115,24 @@ from presentation_api.doors.component_data import ( COMPONENT_DATA_EXCEPTION_EXEMPLARS, COMPONENT_DATA_EXCEPTION_TYPES, + COMPONENT_DATA_PATH, add_component_data_route, classify_component_data_exception, ) from presentation_api.doors.render import ( RENDER_DATA_PATH, - RENDER_ENDPOINT, RENDER_EXCEPTION_EXEMPLARS, RENDER_EXCEPTION_TYPES, RENDER_PATH, build_render_router, classify_render_exception, + replayed, ) from presentation_api.live import build_live_frame_store from presentation_api.models import ComponentReadResponse, CreateReplyResponse, WriteReplyResponse from presentation_api.newest_frames import NewestFrames, resolve_bare_spec from presentation_api.path_params import PathParamInvalid, require_component_id -from presentation_api.refusals import ( - WRITE_ENDPOINT, - binding_invalid_spec, - frame_unavailable_spec, -) +from presentation_api.refusals import binding_invalid_spec, frame_unavailable_spec from presentation_api.reply import serialize_create_reply, serialize_write_reply from presentation_api.retention import stated_retention from presentation_api.vocabulary import LiveVocabularySource @@ -192,14 +196,14 @@ class CreateComponentBody(ComponentBody): def classify_components_exception(exc: BaseException) -> ProblemSpec | None: if isinstance(exc, PathParamInvalid): return ProblemSpec( - status=400, + status=served_status("request_invalid"), code="request_invalid", detail=str(exc), retryable=False, ) if isinstance(exc, CreateRequestInvalid): return ProblemSpec( - status=400, + status=served_status("request_invalid"), code="request_invalid", detail=exc.detail, pointer=exc.pointer, @@ -211,7 +215,7 @@ def classify_components_exception(exc: BaseException) -> ProblemSpec | None: return binding_invalid_spec(exc) if isinstance(exc, ConstraintViolation): return ProblemSpec( - status=400, + status=served_status("constraint_violation"), code="constraint_violation", detail=f"{exc.detail} fix: {exc.fix}", pointer=exc.pointer, @@ -220,28 +224,28 @@ def classify_components_exception(exc: BaseException) -> ProblemSpec | None: ) if isinstance(exc, EditConflict): return ProblemSpec( - status=409, + status=served_status("edit_conflict"), code="edit_conflict", detail=str(exc) or "the component or composition moved before this write committed", retryable=retryable_for("edit_conflict"), ) if isinstance(exc, WriteRefused): return ProblemSpec( - status=403, + status=served_status("write_refused"), code="write_refused", detail=f"{exc.detail} fix: {exc.fix}", retryable=retryable_for("write_refused"), ) if isinstance(exc, DraftNotFound): return ProblemSpec( - status=404, + status=served_status("not_found"), code="not_found", detail=str(exc), retryable=retryable_for("not_found"), ) if isinstance(exc, RevisionNotFound): return ProblemSpec( - status=404, + status=served_status("not_found"), code="not_found", detail=exc.detail, retryable=retryable_for("not_found"), @@ -250,7 +254,7 @@ def classify_components_exception(exc: BaseException) -> ProblemSpec | None: return frame_unavailable_spec(exc) if isinstance(exc, ResourceExhausted): return ProblemSpec( - status=400, + status=served_status("resource_exhausted"), code="resource_exhausted", detail=f"{exc.detail} (limit: {exc.limit_name})", pointer=exc.pointer, @@ -258,14 +262,14 @@ def classify_components_exception(exc: BaseException) -> ProblemSpec | None: ) if isinstance(exc, ReleaseMismatch): return ProblemSpec( - status=503, + status=served_status("release_mismatch"), code="release_mismatch", detail=exc.detail, retryable=retryable_for("release_mismatch"), ) if isinstance(exc, PresentationBackpressure): return ProblemSpec( - status=503, + status=served_status("service_unavailable"), code="service_unavailable", detail="the components door is at its concurrent-write capacity right now; retry", retryable=retryable_for("service_unavailable"), @@ -273,7 +277,7 @@ def classify_components_exception(exc: BaseException) -> ProblemSpec | None: if isinstance(exc, GeneratedIdCollision): logger.warning("Fresh presentation %s ID collision. Retry safely", exc.subject_kind) return ProblemSpec( - status=503, + status=served_status("service_unavailable"), code="service_unavailable", detail=f"A generated presentation {exc.subject_kind} ID collided. Retry the request.", retryable=retryable_for("service_unavailable"), @@ -282,14 +286,14 @@ def classify_components_exception(exc: BaseException) -> ProblemSpec | None: if store_failure_is_permanent(exc.sqlstate): logger.error("Presentation store call failed with a permanent cause: %s", exc) return ProblemSpec( - status=500, + status=served_status("internal_error"), code="internal_error", detail="a presentation store call failed with a non-retryable cause", retryable=retryable_for("internal_error"), ) logger.error("Presentation store call failed: %s", exc) return ProblemSpec( - status=503, + status=served_status("service_unavailable"), code="service_unavailable", detail="a presentation store call failed; retry", retryable=retryable_for("service_unavailable"), @@ -298,25 +302,35 @@ def classify_components_exception(exc: BaseException) -> ProblemSpec | None: # A frame load reads the registry to scope its revocation check, and fails closed. logger.error("Spec registry read scoping a frame load's revocation check failed: %s", exc) return ProblemSpec( - status=503, + status=served_status("service_unavailable"), code="service_unavailable", detail="the spec registry could not scope a frame's revocation check; retry", retryable=retryable_for("service_unavailable"), ) if isinstance(exc, AllowanceExhausted): return ProblemSpec( - status=429, + status=served_status("allowance_exhausted"), code="allowance_exhausted", detail=str(exc), retryable=exc.retryable, headers=exc.headers, ) if isinstance(exc, CallerHeaderInvalid): - return ProblemSpec(status=400, code="request_invalid", detail=str(exc), retryable=False) + return ProblemSpec( + status=served_status("request_invalid"), + code="request_invalid", + detail=str(exc), + retryable=False, + ) if isinstance(exc, CallerUnresolved): # A deployment fault a retry does not clear, so not the retryable service_unavailable. logger.error("Components door cannot name a caller: %s", exc) - return ProblemSpec(status=500, code="internal_error", detail=str(exc), retryable=False) + return ProblemSpec( + status=served_status("internal_error"), + code="internal_error", + detail=str(exc), + retryable=False, + ) if isinstance(exc, DatasetsStoreUnavailable): if store_failure_is_permanent(exc.sqlstate): logger.error( @@ -324,14 +338,14 @@ def classify_components_exception(exc: BaseException) -> ProblemSpec | None: exc, ) return ProblemSpec( - status=500, + status=served_status("internal_error"), code="internal_error", detail="a frame store call failed with a non-retryable cause", retryable=retryable_for("internal_error"), ) logger.error("Datasets store call underlying a frame load failed: %s", exc) return ProblemSpec( - status=503, + status=served_status("service_unavailable"), code="service_unavailable", detail="a frame store call failed; retry", retryable=retryable_for("service_unavailable"), @@ -350,7 +364,7 @@ def _schema_invalid_spec(exc: SchemaInvalid) -> ProblemSpec: f"{exc.detail} minimal legal intent: {json.dumps(exc.minimal_intent, sort_keys=True)}" ) return ProblemSpec( - status=400, + status=served_status("schema_invalid"), code="schema_invalid", detail=detail, pointer=exc.pointer, @@ -358,7 +372,7 @@ def _schema_invalid_spec(exc: SchemaInvalid) -> ProblemSpec: ) legal_fields = cast(tuple[str, ...], exc.legal_fields) return ProblemSpec( - status=400, + status=served_status("schema_invalid"), code="schema_invalid", detail=exc.detail, pointer=exc.pointer, @@ -657,31 +671,24 @@ def job() -> JSONResponse: add_component_data_route(router, deps=deps, run=_run) # Extending the route list keeps each render route's own class, which admits the caller. - router.routes.extend( - build_render_router( - deps=deps, - run=_run, - classify=combine_classifiers( - classify_components_exception, classify_component_data_exception - ), - live_refresh=live_refresh, - ).routes - ) + router.routes.extend(build_render_router(deps=deps, run=_run, live_refresh=live_refresh).routes) return DoorContribution( router=router, - classify=combine_classifiers( - classify_components_exception, - classify_component_data_exception, - classify_render_exception, + classify=replayed( + combine_classifiers( + classify_components_exception, + classify_component_data_exception, + classify_render_exception, + ) ), - # The component read shares the revise route's path, so it runs with - # `presentation.write` bound and serves the write routes' statuses. registry_endpoints={ - COMPONENTS_PATH: WRITE_ENDPOINT, - COMPONENT_PATH: WRITE_ENDPOINT, - RENDER_PATH: RENDER_ENDPOINT, - RENDER_DATA_PATH: RENDER_ENDPOINT, + ("POST", COMPONENTS_PATH): WRITE_ENDPOINT, + ("PATCH", COMPONENT_PATH): WRITE_ENDPOINT, + ("GET", COMPONENT_PATH): COMPONENT_READ_ENDPOINT, + ("GET", COMPONENT_DATA_PATH): COMPONENT_DATA_ENDPOINT, + ("POST", RENDER_PATH): RENDER_ENDPOINT, + ("GET", RENDER_DATA_PATH): RENDER_ENDPOINT, }, exception_types=( PathParamInvalid, diff --git a/src/presentation_api/doors/frames.py b/src/presentation_api/doors/frames.py index f2d09845..61c95079 100644 --- a/src/presentation_api/doors/frames.py +++ b/src/presentation_api/doors/frames.py @@ -35,9 +35,9 @@ from datasets_api.shutdown import ShutdownBudget from datasets_api.store_errors import DatasetsStoreUnavailable from dfo_platform import ProblemSpec -from dfo_platform.doors import DoorContribution +from dfo_platform.doors import DoorContribution, route_registry_endpoints from dfo_platform.store_errors import store_failure_is_permanent -from presentation_api.codes import retryable_for +from presentation_api.codes import FRAMES_ENDPOINT, retryable_for, served_status from presentation_api.config import ( PRESENTATION_FRAMES_MAX_CONCURRENCY, PRESENTATION_FRAMES_TIMEOUT_SECONDS, @@ -65,7 +65,7 @@ class FramesBackpressure(Exception): def classify_frames_exception(exc: BaseException) -> ProblemSpec | None: if isinstance(exc, PathParamInvalid): return ProblemSpec( - status=400, + status=served_status("request_invalid"), code="request_invalid", detail=str(exc), retryable=False, @@ -76,7 +76,7 @@ def classify_frames_exception(exc: BaseException) -> ProblemSpec | None: return frame_unavailable_spec(exc) if isinstance(exc, FramesBackpressure): return ProblemSpec( - status=503, + status=served_status("service_unavailable"), code="service_unavailable", detail="the frames door is at its concurrent-read capacity right now; retry", retryable=retryable_for("service_unavailable"), @@ -85,7 +85,7 @@ def classify_frames_exception(exc: BaseException) -> ProblemSpec | None: # A frame load reads the registry to scope its revocation check, and fails closed. logger.error("Spec registry read scoping a frame load's revocation check failed: %s", exc) return ProblemSpec( - status=503, + status=served_status("service_unavailable"), code="service_unavailable", detail="the spec registry could not scope a frame's revocation check; retry", retryable=retryable_for("service_unavailable"), @@ -97,14 +97,14 @@ def classify_frames_exception(exc: BaseException) -> ProblemSpec | None: exc, ) return ProblemSpec( - status=500, + status=served_status("internal_error"), code="internal_error", detail="a frame store call failed with a non-retryable cause", retryable=retryable_for("internal_error"), ) logger.error("Datasets store call underlying a frame load failed: %s", exc) return ProblemSpec( - status=503, + status=served_status("service_unavailable"), code="service_unavailable", detail="a frame store call failed; retry", retryable=retryable_for("service_unavailable"), @@ -254,6 +254,7 @@ def job() -> JSONResponse: return DoorContribution( router=router, + registry_endpoints=route_registry_endpoints(router, FRAMES_ENDPOINT), classify=classify_frames_exception, exception_types=( PathParamInvalid, diff --git a/src/presentation_api/doors/render.py b/src/presentation_api/doors/render.py index 3c29c94a..67d4e8cd 100644 --- a/src/presentation_api/doors/render.py +++ b/src/presentation_api/doors/render.py @@ -43,18 +43,18 @@ `render_target_mismatch` too. A refusal of the render goes out at the status the code registry gives its code on -`presentation.render` (`RENDER_ENDPOINT`), read through `dfo_platform.codes.registered_status`. +`presentation.render` (`RENDER_ENDPOINT`), which the platform binds while the surface classifies. For five codes it differs from the status the write routes serve: `binding_invalid` and `constraint_violation` answer 422, and `frame_unavailable`, `release_mismatch`, and `resource_exhausted` 409. The caller replays a stored intent it cannot correct, so -`retryable` is the write routes' value read through `presentation_api.codes.replay_retryable`: -true only for a `retry_after_delay` or `refresh_then_retry` code whose occurrence is retryable. +`retryable` is the write routes' value read through `presentation_api.codes.replay_retryable` +(`replayed`, which wraps the components door's classifier): true only for a `retry_after_delay` +or `refresh_then_retry` code whose occurrence is retryable. """ from __future__ import annotations -from collections.abc import Awaitable, Callable, Iterator, Mapping -from contextlib import contextmanager +from collections.abc import Awaitable, Callable, Mapping from typing import TYPE_CHECKING, Literal, cast from fastapi import APIRouter, Path, Query @@ -80,8 +80,8 @@ ) from datasets_api.recipe_store import RecipeReader from dfo_platform import ProblemSpec -from dfo_platform.codes import bound_registry_endpoint, registered_status -from presentation_api.codes import replay_retryable, retryable_for +from dfo_platform.codes import bound_endpoint +from presentation_api.codes import RENDER_ENDPOINT, replay_retryable, retryable_for, served_status from presentation_api.doors.component_data import COMPONENT_DATA_MEDIA_TYPES from presentation_api.models import ComponentReadResponse from presentation_api.path_params import ( @@ -105,8 +105,6 @@ # name decides nothing. _NO_PRINCIPAL = LiveRefreshPrincipal(header="x-dfo-principal", principal="") -RENDER_ENDPOINT = "presentation.render" - class RenderTargetMismatch(Exception): """The target frame may not stand in for the frame the revision is stored on. Maps to @@ -119,27 +117,17 @@ def __init__(self, *, component_id: str, revision: int, detail: str) -> None: super().__init__(f"{component_id} revision {revision}: {detail}") -class RenderRefused(Exception): - """An assembler refusal of a render, as the problem the render routes serve for it.""" - - def __init__(self, spec: ProblemSpec) -> None: - self.spec = spec - super().__init__(spec.detail) - - def classify_render_exception(exc: BaseException) -> ProblemSpec | None: - if isinstance(exc, RenderRefused): - return exc.spec if isinstance(exc, RenderTargetMismatch): return ProblemSpec( - status=409, + status=served_status("render_target_mismatch"), code="render_target_mismatch", detail=str(exc), retryable=retryable_for("render_target_mismatch"), ) if isinstance(exc, ComponentHoldsNoFrame): return ProblemSpec( - status=409, + status=served_status("render_target_mismatch"), code="render_target_mismatch", detail=exc.detail, retryable=retryable_for("render_target_mismatch"), @@ -147,16 +135,27 @@ def classify_render_exception(exc: BaseException) -> ProblemSpec | None: return classify_live_refresh_exception(exc) +def replayed( + classify: Callable[[BaseException], ProblemSpec | None], +) -> Callable[[BaseException], ProblemSpec | None]: + """`classify`, with a refusal on a render route restating `retryable` as its caller, who + replays a stored intent, reads it (`replay_retryable`).""" + + def classify_replayed(exc: BaseException) -> ProblemSpec | None: + spec = classify(exc) + if spec is None or bound_endpoint() != RENDER_ENDPOINT: + return spec + return spec._replace(retryable=replay_retryable(spec.code, retryable=spec.retryable)) + + return classify_replayed + + RENDER_EXCEPTION_TYPES: tuple[type[Exception], ...] = ( - RenderRefused, RenderTargetMismatch, ComponentHoldsNoFrame, RefreshNotPermitted, ) RENDER_EXCEPTION_EXEMPLARS: tuple[BaseException, ...] = ( - RenderRefused( - ProblemSpec(status=409, code="frame_unavailable", detail="exemplar", retryable=True) - ), RenderTargetMismatch(component_id="cmp_exemplar", revision=1, detail="exemplar"), ComponentHoldsNoFrame(component_id="cmp_exemplar", revision=1, detail="exemplar"), RefreshNotPermitted(presented=False), @@ -303,35 +302,14 @@ def build_render_router( *, deps: ComponentsDeps, run: Callable[[Callable[[], Response]], Awaitable[Response]], - classify: Callable[[BaseException], ProblemSpec | None], live_refresh: LiveRefreshPrincipal | None, ) -> APIRouter: """The two render routes, run through the components door's `run` on its `deps`. - - `classify` is the components door's classifier for component and component data refusals, - whose problem the routes restate at its `presentation.render` status and replay - retryability. `live_refresh=None` refuses every caller. + `live_refresh=None` refuses every caller. """ principal = live_refresh if live_refresh is not None else _NO_PRINCIPAL router = APIRouter(route_class=principal.route_class()) - @contextmanager - def restated() -> Iterator[None]: - try: - yield - except Exception as exc: - spec = classify(exc) or classify_render_exception(exc) - if spec is None: - raise - with bound_registry_endpoint(RENDER_ENDPOINT): - status = registered_status(spec.code) - raise RenderRefused( - spec._replace( - status=status, - retryable=replay_retryable(spec.code, retryable=spec.retryable), - ) - ) from exc - def accepted_revision(*, component_id: str, revision: int, target: FrameRef) -> RevisionRecord: record = read_stored_revision( component_id=component_id, revision=revision, revision_store=deps.revision_store @@ -350,19 +328,16 @@ async def render_component_revision( ) def job() -> JSONResponse: - with restated(): - record = accepted_revision( - component_id=component_id, revision=revision, target=target - ) - reply = render_component( - component_id=component_id, - revision=revision, - target_frame=target, - frame_store=deps.frame_store, - revision_store=deps.revision_store, - release_registry=deps.release_registry, - vocabulary_source=deps.vocabulary_source, - ) + record = accepted_revision(component_id=component_id, revision=revision, target=target) + reply = render_component( + component_id=component_id, + revision=revision, + target_frame=target, + frame_store=deps.frame_store, + revision_store=deps.revision_store, + release_registry=deps.release_registry, + vocabulary_source=deps.vocabulary_source, + ) retention = stated_retention(target, snapshot_reader=deps.snapshot_reader) return JSONResponse( { @@ -398,19 +373,18 @@ async def read_rendered_component_data( target = _target_frame(spec_id=spec_id, content_version=content_version) def job() -> Response: - with restated(): - accepted_revision(component_id=component_id, revision=revision, target=target) - exported = export_component_data( - component_id=component_id, - revision=revision, - format=format_, - bundle=bundle, - revision_store=deps.revision_store, - frame_store=deps.frame_store, - site_url=deps.site.public_site_url, - target_frame=target, - release_registry=deps.release_registry, - ) + accepted_revision(component_id=component_id, revision=revision, target=target) + exported = export_component_data( + component_id=component_id, + revision=revision, + format=format_, + bundle=bundle, + revision_store=deps.revision_store, + frame_store=deps.frame_store, + site_url=deps.site.public_site_url, + target_frame=target, + release_registry=deps.release_registry, + ) return Response( content=exported.data, media_type=exported.media_type, diff --git a/src/presentation_api/doors/shares.py b/src/presentation_api/doors/shares.py index a2fadccc..8e711360 100644 --- a/src/presentation_api/doors/shares.py +++ b/src/presentation_api/doors/shares.py @@ -209,6 +209,7 @@ from dfo_platform.doors import DoorContribution, route_registry_endpoints from dfo_platform.state import StateSlot from materialise.formats import ExcelIllegalCharacterError +from presentation_api.codes import SHARES_ENDPOINT, served_status from presentation_api.config import ( PRESENTATION_COMPONENTS_MAX_CONCURRENCY, PRESENTATION_COMPONENTS_TIMEOUT_SECONDS, @@ -283,7 +284,6 @@ logger = logging.getLogger(__name__) -SHARES_ENDPOINT = "presentation.shares" """The code registry endpoint every share route serves.""" SHARES_PATH = "/api/presentation/shares" @@ -1134,33 +1134,54 @@ def classify_shares_exception(exc: BaseException) -> ProblemSpec | None: the share routes share with it.""" if isinstance(exc, WriteTokenRequired): return ProblemSpec( - status=403, code="write_token_required", detail=str(exc), retryable=False + status=served_status("write_token_required"), + code="write_token_required", + detail=str(exc), + retryable=False, ) if isinstance(exc, WriteTokenInvalid): - return ProblemSpec(status=403, code="write_token_invalid", detail=str(exc), retryable=False) + return ProblemSpec( + status=served_status("write_token_invalid"), + code="write_token_invalid", + detail=str(exc), + retryable=False, + ) if isinstance(exc, ShareNotReplaceable): return ProblemSpec( - status=409, code="share_not_replaceable", detail=str(exc), retryable=False + status=served_status("share_not_replaceable"), + code="share_not_replaceable", + detail=str(exc), + retryable=False, ) if isinstance(exc, ShareIdTaken): - return ProblemSpec(status=409, code="share_id_taken", detail=str(exc), retryable=False) + return ProblemSpec( + status=served_status("share_id_taken"), + code="share_id_taken", + detail=str(exc), + retryable=False, + ) if isinstance(exc, ShareImportNotPermitted): return ProblemSpec( - status=403, code="share_import_not_permitted", detail=str(exc), retryable=False + status=served_status("share_import_not_permitted"), + code="share_import_not_permitted", + detail=str(exc), + retryable=False, ) if isinstance(exc, SlotDrawsNoFrame): return ProblemSpec( - status=400, + status=served_status("request_invalid"), code="request_invalid", detail=str(exc), pointer=f"/slots/{exc.index}/frame", retryable=False, ) if isinstance(exc, ShareNotFound | ShareSlotEmpty): - return ProblemSpec(status=404, code="not_found", detail=str(exc), retryable=False) + return ProblemSpec( + status=served_status("not_found"), code="not_found", detail=str(exc), retryable=False + ) if isinstance(exc, AllowanceExhausted): return ProblemSpec( - status=429, + status=served_status("allowance_exhausted"), code="allowance_exhausted", detail=str(exc), retryable=exc.retryable, @@ -1168,23 +1189,43 @@ def classify_shares_exception(exc: BaseException) -> ProblemSpec | None: ) if isinstance(exc, PinBudgetExhausted | OpenPinBudgetExhausted): return ProblemSpec( - status=503, + status=served_status("service_unavailable"), code="service_unavailable", detail=str(exc), retryable=True, headers=exc.headers, ) if isinstance(exc, CallerHeaderInvalid): - return ProblemSpec(status=400, code="request_invalid", detail=str(exc), retryable=False) + return ProblemSpec( + status=served_status("request_invalid"), + code="request_invalid", + detail=str(exc), + retryable=False, + ) if isinstance(exc, CallerUnresolved): # A deployment fault a retry does not clear. This surface's `service_unavailable` is # retryable, so it answers `internal_error`. logger.error("Shares door cannot name a caller: %s", exc) - return ProblemSpec(status=500, code="internal_error", detail=str(exc), retryable=False) + return ProblemSpec( + status=served_status("internal_error"), + code="internal_error", + detail=str(exc), + retryable=False, + ) if isinstance(exc, SharePayloadTooLarge): - return ProblemSpec(status=413, code="payload_too_large", detail=str(exc), retryable=False) + return ProblemSpec( + status=served_status("payload_too_large"), + code="payload_too_large", + detail=str(exc), + retryable=False, + ) if isinstance(exc, SharesUnavailable): - return ProblemSpec(status=503, code="service_unavailable", detail=str(exc), retryable=True) + return ProblemSpec( + status=served_status("service_unavailable"), + code="service_unavailable", + detail=str(exc), + retryable=True, + ) return classify_components_exception(exc) or classify_component_data_exception(exc) diff --git a/src/presentation_api/newest_frames.py b/src/presentation_api/newest_frames.py index a9a2abdc..aa44415d 100644 --- a/src/presentation_api/newest_frames.py +++ b/src/presentation_api/newest_frames.py @@ -29,7 +29,7 @@ served definition's newest version in the root's default rendering, rebuilt when its head is stale. A rebuild too large for the call's budget (`doors.definition.inline_definition_deps`) goes to an operation, so a retry finds it, and the call answers `NewestPending`. -`resolve_bare_spec` turns its refusals into the `frame_unavailable` the presentation doors serve. +`resolve_bare_spec` turns its refusals into the `FrameUnavailable` the presentation doors refuse. `DatasetsNewestFrames` reads the Datasets surface's `DepsCell`, so it answers only in an app that mounts that surface (`dfo_apis.app` passes the cell and the `RunningAnalyses` to both). @@ -202,7 +202,7 @@ def resolve_bare_spec(spec_id: str, *, newest: NewestFrames | None) -> FrameRef: """The frame a bare `spec_id` names (`NewestFrames.newest_of_spec`). Raises: - FrameUnavailable: `io` (503, retryable) while the version builds or for a refusal a + FrameUnavailable: `pending` (retryable) while the version builds or for a refusal a retry may clear; `quarantined` for a quarantined identity; `revoked` for a revoked version; `unknown` (404) for any other refusal below status 500, the Datasets code and detail in its detail; and @@ -227,7 +227,7 @@ def resolve_bare_spec(spec_id: str, *, newest: NewestFrames | None) -> FrameRef: if problem is None: raise if problem.retryable: - reason = "io" + reason = "pending" elif problem.code == "identity_quarantined": reason = "quarantined" elif problem.code == "snapshot_revoked": @@ -244,7 +244,7 @@ def resolve_bare_spec(spec_id: str, *, newest: NewestFrames | None) -> FrameRef: ) from exc if isinstance(resolved, NewestPending): raise FrameUnavailable( - reason="io", + reason="pending", detail=f"the newest version of {spec_id} is building; retry in " f"{math.ceil(resolved.retry_after_s)} s", ) diff --git a/src/presentation_api/refusals.py b/src/presentation_api/refusals.py index 85dd7a56..a4ad44da 100644 --- a/src/presentation_api/refusals.py +++ b/src/presentation_api/refusals.py @@ -5,25 +5,25 @@ from __future__ import annotations +import logging +from typing import get_args + from fastapi.exceptions import RequestValidationError from assembler.errors import BindingInvalid, FrameUnavailable +from assembler.ports import FrameUnavailableReason from dfo_platform import Alternative, Cause, ProblemIssue, ProblemSpec -from dfo_platform.codes import bound_registry_endpoint, registered_status from dfo_platform.validation import validation_problem -from presentation_api.codes import retryable_for +from presentation_api.codes import retryable_for, served_status __all__ = [ "EXPIRED_FRAME_FIX", - "WRITE_ENDPOINT", "binding_invalid_spec", "frame_unavailable_spec", "request_validation_spec", ] -# The code registry endpoint of the card writes. The registry maps no endpoint onto the read -# routes, which share the write routes' classifiers and so their statuses. -WRITE_ENDPOINT = "presentation.write" +logger = logging.getLogger(__name__) def binding_invalid_spec(exc: BindingInvalid) -> ProblemSpec: @@ -44,7 +44,7 @@ def binding_invalid_spec(exc: BindingInvalid) -> ProblemSpec: for dimension in exc.structural_view.dimensions ] return ProblemSpec( - status=400, + status=served_status("binding_invalid"), code="binding_invalid", detail=exc.detail, pointer=exc.pointer, @@ -58,31 +58,36 @@ def binding_invalid_spec(exc: BindingInvalid) -> ProblemSpec: ) -def _outcome(code: str, *, retryable: bool) -> tuple[str, int, bool]: - """`code` and `retryable` with the status the code registry gives `code` on the write - routes.""" - with bound_registry_endpoint(WRITE_ENDPOINT): - return code, registered_status(code), retryable - - -# The code, status, and retryability each `FrameUnavailable` reason refuses with. `expired` and -# `ops_unsupported` can succeed after republishing. Revoked, missing, digest mismatch, -# quarantined, and unknown refs are permanent. A store that fails to answer (`io`), a derived -# frame whose recipe is not written yet (`recipe_pending`), which a recipe write lets a retry pin, -# and a recipe the store cannot read (`recipe_unreadable`) are `service_unavailable`. -_FRAME_UNAVAILABLE_OUTCOME: dict[str, tuple[str, int, bool]] = { - "expired": _outcome("frame_unavailable", retryable=True), - "revoked": _outcome("frame_unavailable", retryable=False), - "ops_unsupported": _outcome("frame_unavailable", retryable=True), - "missing_rendering": _outcome("frame_unavailable", retryable=False), - "digest_mismatch": _outcome("frame_unavailable", retryable=False), - "quarantined": _outcome("frame_unavailable", retryable=False), - "io": _outcome("service_unavailable", retryable=True), - "recipe_pending": _outcome("service_unavailable", retryable=True), - "recipe_unreadable": _outcome("service_unavailable", retryable=True), - "unknown": _outcome("frame_unavailable", retryable=False), +# The code and retryability each `FrameUnavailable` reason refuses with. `expired`, +# `ops_unsupported` and `recipe_pending` can succeed once the frame is republished or its recipe +# written. Revoked, missing, digest mismatch, quarantined, and unknown refs are permanent. A store +# that fails to answer (`io`) and a newest version not ready to serve (`pending`) are +# `service_unavailable`. A recipe the store cannot read (`recipe_unreadable`) is write-once, so no +# retry clears it. +_FRAME_UNAVAILABLE_OUTCOME: dict[FrameUnavailableReason, tuple[str, bool]] = { + "expired": ("frame_unavailable", True), + "revoked": ("frame_unavailable", False), + "ops_unsupported": ("frame_unavailable", True), + "missing_rendering": ("frame_unavailable", False), + "digest_mismatch": ("frame_unavailable", False), + "quarantined": ("frame_unavailable", False), + "recipe_pending": ("frame_unavailable", True), + "unknown": ("frame_unavailable", False), + "io": ("service_unavailable", True), + "pending": ("service_unavailable", True), + "recipe_unreadable": ("internal_error", False), } +if set(_FRAME_UNAVAILABLE_OUTCOME) != set(get_args(FrameUnavailableReason)): + # Fail at import if a reason and its outcome drift apart. + raise AssertionError( + f"_FRAME_UNAVAILABLE_OUTCOME {sorted(_FRAME_UNAVAILABLE_OUTCOME)} does not match " + f"FrameUnavailableReason {sorted(get_args(FrameUnavailableReason))}" + ) + +# The reasons that name a server fault. +_LOGGED_REASONS: frozenset[str] = frozenset({"io", "recipe_unreadable"}) + # An expired frame aged out of the working tier. A card made on it draws again from current # data, which a fetch of its ref with latest=True returns. @@ -93,17 +98,19 @@ def _outcome(code: str, *, retryable: bool) -> tuple[str, int, bool]: def frame_unavailable_spec(exc: FrameUnavailable) -> ProblemSpec: - """The refusal `_FRAME_UNAVAILABLE_OUTCOME` gives `exc.reason`. A `frame_unavailable` names - the reason as `cause.code`, so a client tells a frame it can rebuild from one taken down, and - an expired frame's detail ends with `EXPIRED_FRAME_FIX`.""" - code, status, retryable = _FRAME_UNAVAILABLE_OUTCOME[exc.reason] + """The refusal `_FRAME_UNAVAILABLE_OUTCOME` gives `exc.reason`, at the bound endpoint's status, + with the reason as `cause.code`, so a client tells a frame it can rebuild from one taken + down. An expired frame's detail ends with `EXPIRED_FRAME_FIX`.""" + code, retryable = _FRAME_UNAVAILABLE_OUTCOME[exc.reason] + if exc.reason in _LOGGED_REASONS: + logger.error("Frame refused for reason %s: %s", exc.reason, exc.detail) detail = f"{exc.detail}; {EXPIRED_FRAME_FIX}" if exc.reason == "expired" else exc.detail return ProblemSpec( - status=status, + status=served_status(code), code=code, detail=detail, retryable=retryable, - cause=Cause(code=exc.reason) if code == "frame_unavailable" else None, + cause=Cause(code=exc.reason), ) diff --git a/src/presentation_api/version.py b/src/presentation_api/version.py index 5b0f8caf..f6d7e885 100644 --- a/src/presentation_api/version.py +++ b/src/presentation_api/version.py @@ -1,8 +1,9 @@ """`compute_presentation_capability_version` returns `pres_` plus 16 SHA-256 hex digits for the release this surface serves. The hash covers the release document with a fixed `catalog_version`, strict intent schemas, formatter profile, grammar members, encoding classes, -constraints, rendered refusal exemplars, and frame-unavailable outcomes. Wording in the release -schemas affects the hash. +constraints, rendered refusal exemplars, frame-unavailable outcomes, and the status the code +registry gives each code on each `presentation.*` endpoint. Wording in the release schemas +affects the hash. """ from __future__ import annotations @@ -13,9 +14,14 @@ from collections.abc import Mapping from assembler.catalogue import CatalogueRelease +from assembler.ports import FrameUnavailableReason from dfo_platform import Alternative, CodeCatalogue, Envelope, ProblemIssue from dfo_platform.trace import set_trace_id -from presentation_api.codes import CODE_RETRY_MODE, PRESENTATION_CODE_CATALOGUE +from presentation_api.codes import ( + CODE_RETRY_MODE, + PRESENTATION_CODE_CATALOGUE, + presentation_statuses, +) from presentation_api.doors.release import release_document from presentation_api.refusals import _FRAME_UNAVAILABLE_OUTCOME @@ -72,9 +78,13 @@ def compute_presentation_capability_version( release: CatalogueRelease, code_catalogue: CodeCatalogue = PRESENTATION_CODE_CATALOGUE, retry_modes: Mapping[str, str] = CODE_RETRY_MODE, - frame_unavailable_outcome: Mapping[str, tuple[str, int, bool]] = _FRAME_UNAVAILABLE_OUTCOME, + frame_unavailable_outcome: Mapping[ + FrameUnavailableReason, tuple[str, bool] + ] = _FRAME_UNAVAILABLE_OUTCOME, + registry_statuses: Mapping[str, Mapping[str, int]] | None = None, ) -> str: """Return the capability hash for `release` and the supplied refusal data.""" + statuses = presentation_statuses() if registry_statuses is None else registry_statuses refusal_exemplars = contextvars.copy_context().run( _render_refusal_exemplars, code_catalogue, retry_modes ) @@ -95,6 +105,7 @@ def compute_presentation_capability_version( "frame_unavailable_outcome": { reason: list(outcome) for reason, outcome in frame_unavailable_outcome.items() }, + "registry_statuses": {endpoint: dict(codes) for endpoint, codes in statuses.items()}, "release_document": pinned_release_document, } canonical = json.dumps(payload, sort_keys=True, separators=(",", ":")) diff --git a/src/presentation_store/README.md b/src/presentation_store/README.md index 9105a83d..d37c5e70 100644 --- a/src/presentation_store/README.md +++ b/src/presentation_store/README.md @@ -153,7 +153,7 @@ A share id with no row, or a taken-down one, raises `ShareNotFound` from every m 1. Promotes the canonical artifact from `working` to `pinned` tier in `ArtifactStore` when currently in the working tier. 2. Inserts an artifact reference record (`reference_kind="presentation_draft"`, `principal="presentation:"`) into `datasets.artifact_reference` using `ArtifactReferenceWriter`. -3. For a derived (`ops_`) frame, pins each root version its recipe names (each root's `dataset`), and theirs in turn. A derived frame with no recipe yet refuses the retryable `recipe_pending`, since a later recipe write lets a retry pin its roots. A derived frame with no recipe that a reference row already names (`ArtifactReferenceWriter.holds_reference`) pins alone, since every pin reads a derived frame's recipe before it writes a reference and the datasets pin door names `spec_` ids alone, so such a frame was pinned before recipes were stored. A share of it draws the frame's stored rows, and a live slot cannot draw it on newer data and states `stale`. The recipe is read again once the reference is found, and a recipe written for the frame later (`PUT /api/datasets/frames/{spec_id}/{content_version}/recipe`) makes the next pin name its roots. A store with no recipe reader, or a recipe root naming no dataset version, refuses the retryable `recipe_unreadable`. The recipes are read first, so either refusal pins nothing. A root that has expired or been revoked is left unpinned and logged, and does not block a share. +3. For a derived (`ops_`) frame, pins each root version its recipe names (each root's `dataset`), and theirs in turn. A derived frame with no recipe yet refuses the retryable `recipe_pending`, since a later recipe write lets a retry pin its roots. A derived frame with no recipe that a reference row already names (`ArtifactReferenceWriter.holds_reference`) pins alone, since every pin reads a derived frame's recipe before it writes a reference and the datasets pin door names `spec_` ids alone, so such a frame was pinned before recipes were stored. A share of it draws the frame's stored rows, and a live slot cannot draw it on newer data and states `stale`. The recipe is read again once the reference is found, and a recipe written for the frame later (`PUT /api/datasets/frames/{spec_id}/{content_version}/recipe`) makes the next pin name its roots. A store with no recipe reader, or a recipe root naming no dataset version, refuses `recipe_unreadable`, which no retry clears. The recipes are read first, so either refusal pins nothing. A root that has expired or been revoked is left unpinned and logged, and does not block a share. No create, revise, or render pins. `pin_card_frames(component_id, revision, revision_store, frame_store)` pins the frame a card revision draws, and a share calls it for each card it first includes. diff --git a/src/presentation_store/frame_store.py b/src/presentation_store/frame_store.py index e9f62bd5..210a0276 100644 --- a/src/presentation_store/frame_store.py +++ b/src/presentation_store/frame_store.py @@ -158,7 +158,7 @@ from contextlib import contextmanager from contextvars import ContextVar from dataclasses import dataclass, replace -from typing import Protocol, cast +from typing import Protocol, TypeGuard, cast import pyarrow as pa import pyarrow.parquet as pq @@ -398,22 +398,46 @@ def _io_failure(*, action: str, digest: str, exc: Exception) -> FrameUnavailable ) +def _is_frame_document(doc: object) -> TypeGuard[Mapping[str, object]]: + """Whether `doc`, parsed from a frame1 document, has the shape every reader of a frame + takes: named columns, and rows of one cell per column.""" + if not isinstance(doc, dict): + return False + columns, rows = doc.get("columns"), doc.get("rows") + return ( + isinstance(columns, list) + and all(isinstance(column, str) for column in columns) + and isinstance(rows, list) + and all(isinstance(row, list) and len(row) == len(columns) for row in rows) + ) + + def _decode_frame( data: bytes, *, location: StoredArtifactLocation, max_rows: int ) -> tuple[Mapping[str, object], int]: """The frame1 document `data` holds and the bytes the cache counts for it: a frame1 document parsed, at its byte length, or a canonical frame built into one, at `_document_bytes`. A canonical frame whose Parquet footer counts more than `max_rows` rows - refuses `missing_rendering` before any row is decoded. Bytes that do not decode never will, - so they refuse `missing_rendering` too.""" + refuses `missing_rendering` before any row is decoded. + + `data` is in memory and passed its digest check, so a decoder's verdict on it is final. Bytes + the decoders reject as malformed refuse `missing_rendering`: `ValueError`, which covers + `pa.ArrowInvalid` and malformed JSON; `OSError`, which is how pyarrow reports a corrupt footer, + page, or column chunk, and no read here touches a file; `RecursionError` from JSON nested past + the parser's depth; and a parsed document that is not a frame. Any other Arrow failure and + `MemoryError` say nothing about the bytes, since an allocation failure or a codec this build + lacks clears on another instance or release, so they refuse the retryable `io`.""" try: if location.media_type == FRAME1_MEDIA_TYPE: - return cast("Mapping[str, object]", json.loads(data)), len(data) + parsed = json.loads(data) + if not _is_frame_document(parsed): + raise ValueError("not a frame1 document") + return parsed, len(data) rows = pq.read_metadata(pa.BufferReader(data)).num_rows if rows > max_rows: raise _over_row_bound(location, rows=rows, max_rows=max_rows) doc = canonical_frame_document(read_canonical_frame_table(data)) - except (ValueError, pa.ArrowException) as exc: + except (ValueError, OSError, RecursionError) as exc: raise FrameUnavailable( reason="missing_rendering", detail=( @@ -421,6 +445,8 @@ def _decode_frame( f"{type(exc).__name__}" ), ) from None + except (MemoryError, pa.ArrowException) as exc: + raise _io_failure(action="decoding", digest=location.artifact_digest, exc=exc) from None rows = cast("Sequence[Sequence[object]]", doc["rows"]) return doc, _document_bytes(rows=len(rows), columns=len(cast("Sequence[str]", doc["columns"]))) @@ -1253,7 +1279,7 @@ def pin(self, *, frame_ref: FrameRef, component_id: str, caller: Caller | None = write, makes a retry pin the roots. A derived frame with no recipe that a reference already names pins with no roots (`_recipe_roots`). A store with no recipe reader, and a recipe root that names no dataset version, refuse `recipe_unreadable`, a server - fault. Both answer a retryable 503 (`presentation_api.refusals`). Then the frame pins, + fault no retry clears (`presentation_api.refusals`). Then the frame pins, and a refusal there pins nothing either. Then each root pins while it is still held: a root that aged out of the working tier, was revoked, or cannot be read is left unpinned and logged. A root whose pin fails on I/O raises `io`, and a retry pins what is left. diff --git a/src/search/engine/app.py b/src/search/engine/app.py index 17249bda..0b5ee7c7 100644 --- a/src/search/engine/app.py +++ b/src/search/engine/app.py @@ -1249,8 +1249,8 @@ async def lifespan(app: FastAPI) -> AsyncIterator[None]: ), lifespan=lifespan, registry_endpoints={ - "/api/search/resolve": "search.resolve", - "/api/search/options": "search.options", + ("POST", "/api/search/resolve"): "search.resolve", + ("POST", "/api/search/options"): "search.options", }, request_scope=lambda: _request_scope(catalogue, release, conversions, variable_breadth), ) diff --git a/src/skills_api/app.py b/src/skills_api/app.py index 193336cc..66463589 100644 --- a/src/skills_api/app.py +++ b/src/skills_api/app.py @@ -85,6 +85,6 @@ async def lifespan(_app: FastAPI): ), lifespan=lifespan, registry_endpoints={ - path: endpoint for door in doors for path, endpoint in door.registry_endpoints.items() + key: endpoint for door in doors for key, endpoint in door.registry_endpoints.items() }, ) diff --git a/tests/code_registry.py b/tests/code_registry.py new file mode 100644 index 00000000..14e5e248 --- /dev/null +++ b/tests/code_registry.py @@ -0,0 +1,53 @@ +"""What the code registry maps, read the way the refusal sweeps compare a door against it.""" + +from __future__ import annotations + +from dfo_spec_kit.registry import REGISTRY, CodeMapping + +from dfo_platform.app import Surface +from dfo_platform.doors import RouteKey, route_keys + + +def _problem_mappings(code: str) -> tuple[CodeMapping, ...]: + entry = REGISTRY[code] if code in REGISTRY else None # noqa: SIM401 -- CodeRegistry has no .get() + mappings = entry.mappings if entry is not None else () + return tuple( + mapping + for mapping in mappings + if mapping.channel == "problem_details" and mapping.http_status is not None + ) + + +def problem_statuses(code: str, endpoint: str) -> set[int]: + """The statuses the registry gives `code`'s problem details on `endpoint`.""" + return { + status + for mapping in _problem_mappings(code) + if mapping.endpoint == endpoint and (status := mapping.http_status) is not None + } + + +def registered_status_codes(endpoint: str) -> frozenset[tuple[str, int]]: + """The active `problem_details` code-status pairs on `endpoint`. Manifest warnings use a + separate channel without HTTP statuses.""" + return frozenset( + (entry.code, status) + for entry in REGISTRY.entries + if entry.lifecycle == "active" + for status in problem_statuses(entry.code, endpoint) + ) + + +def registered_endpoints(prefix: str) -> set[str]: + """Every registry endpoint whose name starts with `prefix`.""" + return { + mapping.endpoint + for entry in REGISTRY.entries + for mapping in entry.mappings + if mapping.endpoint.startswith(prefix) + } + + +def unbound_routes(surface: Surface) -> frozenset[RouteKey]: + """The surface's routes that name no registry endpoint, one key per method.""" + return route_keys(surface.router) - set(surface.registry_endpoints) diff --git a/tests/contract/test_client_contract.py b/tests/contract/test_client_contract.py index 0d7e0d34..fa4df067 100644 --- a/tests/contract/test_client_contract.py +++ b/tests/contract/test_client_contract.py @@ -167,7 +167,7 @@ def _route(served: _Served, method: str, path: str) -> _Route | None: return _Route( path=schema_path, method=method.lower(), - endpoint=surface.registry_endpoints.get(route.path), + endpoint=surface.registry_endpoints.get((method, route.path)), operation=operation, ) return None diff --git a/tests/datasets_api/test_refusal_registry.py b/tests/datasets_api/test_refusal_registry.py index f8415a91..9e0c40b2 100644 --- a/tests/datasets_api/test_refusal_registry.py +++ b/tests/datasets_api/test_refusal_registry.py @@ -36,9 +36,10 @@ from datasets_api.operation_store import classify_operation_store_exception from datasets_api.store_errors import classify_store_exception from dfo_platform.codes import bound_registry_endpoint, registered_status -from dfo_platform.doors import DoorContribution, combine_classifiers +from dfo_platform.doors import DoorContribution, combine_classifiers, route_keys from dfo_platform.problem import Refusal from spec_registry import IdentityQuarantined +from tests.code_registry import problem_statuses # The registry endpoints each door's routes serve. _ENDPOINTS = { @@ -99,18 +100,6 @@ def _subclasses(cls: type) -> set[type]: return found -def _statuses(code: str, endpoint: str) -> set[int]: - """The statuses the registry gives `code` on `endpoint`'s problem details.""" - entry = REGISTRY[code] if code in REGISTRY else None # noqa: SIM401 -- CodeRegistry has no .get() - return { - mapping.http_status - for mapping in (entry.mappings if entry is not None else ()) - if mapping.endpoint == endpoint - and mapping.channel == "problem_details" - and mapping.http_status is not None - } - - def _inventories(doors: dict[str, DoorContribution]) -> dict[str, dict[str, tuple[type, ...]]]: """Each door's routes, by registry endpoint, with the refusal classes each declares.""" return { @@ -145,7 +134,7 @@ def test_every_declared_refusal_has_a_registered_status_on_each_endpoint_raising (cls.code, endpoint) for cls, endpoints in _raised(doors).items() for endpoint in endpoints - if not _statuses(cls.code, endpoint) + if not problem_statuses(cls.code, endpoint) } assert sorted(missing) == [] @@ -158,7 +147,7 @@ def test_every_declared_refusal_code_has_one_status_on_each_endpoint_declaring_i (cls.code, endpoint) for cls, endpoints in _raised(doors).items() for endpoint in endpoints - if len(_statuses(cls.code, endpoint)) > 1 + if len(problem_statuses(cls.code, endpoint)) > 1 } assert sorted(ambiguous) == [] unresolved = [] @@ -189,13 +178,11 @@ def test_every_datasets_route_binds_the_registry_endpoint_its_door_serves(doors) for name, door in doors.items(): declared = set(door.registry_endpoints.values()) assert declared == set(_ENDPOINTS[name]), name - routed = {getattr(route, "path", None) for route in door.router.routes} - assert set(door.registry_endpoints) <= routed, name + assert set(door.registry_endpoints) <= route_keys(door.router), name assert all( - getattr(route, "path", None) in door.registry_endpoints + route_keys(door.router) == set(door.registry_endpoints) for name, door in doors.items() if name != "meta" - for route in door.router.routes ) @@ -231,7 +218,7 @@ def test_every_refusal_exemplar_goes_out_at_its_registered_status(doors): disagreements = { (code, endpoint, status) for code, endpoint, status in sent - if _statuses(code, endpoint) != {status} + if problem_statuses(code, endpoint) != {status} } assert sorted(disagreements) == [] @@ -292,6 +279,6 @@ def test_every_exemplar_a_route_raises_maps_on_its_endpoint_under_the_surface_cl except LookupError: unmapped.add((type(exemplar).__qualname__, endpoint)) continue - if spec is not None and _statuses(spec.code, endpoint) != {spec.status}: + if spec is not None and problem_statuses(spec.code, endpoint) != {spec.status}: unmapped.add((type(exemplar).__qualname__, endpoint)) assert sorted(unmapped) == [] diff --git a/tests/engine/test_refusal_registry.py b/tests/engine/test_refusal_registry.py index 8e7ab66c..2291e196 100644 --- a/tests/engine/test_refusal_registry.py +++ b/tests/engine/test_refusal_registry.py @@ -5,10 +5,9 @@ from __future__ import annotations -from dfo_spec_kit.registry import REGISTRY - from dfo_platform.codes import bound_registry_endpoint from search.engine.app import search_surface +from tests.code_registry import problem_statuses, registered_endpoints, unbound_routes # Routes that serve no registry endpoint: they refuse only with platform codes. _UNBOUND_PATHS = frozenset( @@ -21,32 +20,14 @@ ) -def _statuses(code: str, endpoint: str) -> set[int]: - """The statuses the registry gives `code`'s problem details on `endpoint`.""" - entry = REGISTRY[code] if code in REGISTRY else None # noqa: SIM401 -- CodeRegistry has no .get() - return { - mapping.http_status - for mapping in (entry.mappings if entry is not None else ()) - if mapping.channel == "problem_details" - and mapping.http_status is not None - and mapping.endpoint == endpoint - } - - def test_every_search_route_binds_the_registry_endpoint_it_serves(): """The surface binds every Search endpoint the registry maps, each to a route it serves, and leaves only the routes in `_UNBOUND_PATHS` unbound.""" surface = search_surface() - registered = { - mapping.endpoint - for entry in REGISTRY.entries - for mapping in entry.mappings - if mapping.endpoint.startswith("search.") - } - routed = {getattr(route, "path", None) for route in surface.router.routes} + registered = registered_endpoints("search.") assert set(surface.registry_endpoints.values()) == registered - assert routed - set(surface.registry_endpoints) == _UNBOUND_PATHS + assert {path for _, path in unbound_routes(surface)} == _UNBOUND_PATHS def test_every_search_exemplar_goes_out_at_its_registered_status(): @@ -64,7 +45,7 @@ def test_every_search_exemplar_goes_out_at_its_registered_status(): spec = surface.classify(exemplar) if spec is None: continue - statuses = _statuses(spec.code, endpoint) + statuses = problem_statuses(spec.code, endpoint) if not statuses: continue mapped = True diff --git a/tests/graph_api/test_refusal_registry.py b/tests/graph_api/test_refusal_registry.py index ef161685..bc24fdde 100644 --- a/tests/graph_api/test_refusal_registry.py +++ b/tests/graph_api/test_refusal_registry.py @@ -8,7 +8,6 @@ from __future__ import annotations import pytest -from dfo_spec_kit.registry import REGISTRY from dfo_platform.codes import bound_registry_endpoint from dfo_platform.doors import DoorContribution @@ -17,6 +16,7 @@ from graph_api.doors.node import build_node_door from graph_api.doors.resolve import build_resolve_door from graph_api.doors.sources import build_sources_door +from tests.code_registry import problem_statuses, registered_endpoints, unbound_routes from .conftest import FakeGraphNodeReader @@ -34,32 +34,17 @@ def doors() -> tuple[DoorContribution, ...]: def _statuses(code: str, endpoint: str) -> set[int]: """The statuses the registry gives `code` on `endpoint`'s problem details, or on `platform.any`'s when `endpoint` maps none.""" - entry = REGISTRY[code] if code in REGISTRY else None # noqa: SIM401 -- CodeRegistry has no .get() - mappings = [ - mapping - for mapping in (entry.mappings if entry is not None else ()) - if mapping.channel == "problem_details" and mapping.http_status is not None - ] - on_endpoint = {mapping.http_status for mapping in mappings if mapping.endpoint == endpoint} - return on_endpoint or { - mapping.http_status for mapping in mappings if mapping.endpoint == "platform.any" - } + return problem_statuses(code, endpoint) or problem_statuses(code, "platform.any") def test_every_graph_route_binds_the_registry_endpoint_it_serves(): """The surface binds every Graph endpoint the registry maps, each to a route it serves, and leaves only the routes in `_UNBOUND_PATHS` unbound.""" surface = graph_surface(graph=FakeGraphNodeReader()) - registered = { - mapping.endpoint - for entry in REGISTRY.entries - for mapping in entry.mappings - if mapping.endpoint.startswith("graph.") - } - routed = {getattr(route, "path", None) for route in surface.router.routes} + registered = registered_endpoints("graph.") assert set(surface.registry_endpoints.values()) == registered - assert routed - set(surface.registry_endpoints) == _UNBOUND_PATHS + assert {path for _, path in unbound_routes(surface)} == _UNBOUND_PATHS def test_every_graph_exemplar_goes_out_at_its_registered_status(doors): diff --git a/tests/platform/test_registry_binding.py b/tests/platform/test_registry_binding.py index b07f614b..9c445d0d 100644 --- a/tests/platform/test_registry_binding.py +++ b/tests/platform/test_registry_binding.py @@ -39,6 +39,7 @@ from dfo_platform.doors import DoorContribution from search.engine.config import SEARCH_RESOLVE_DOORS from search.models import NoDataReason +from tests.code_registry import registered_status_codes _REGISTRY = load_registry() @@ -220,25 +221,9 @@ def _served_status_codes( return frozenset(outcomes) -def _registered_status_codes(endpoint: str) -> frozenset[tuple[str, int]]: - """Returns active `problem_details` code-status pairs for `endpoint`. - - Manifest warnings use a separate channel without HTTP statuses.""" - pairs: set[tuple[str, int]] = set() - for entry in _REGISTRY.entries: - if entry.lifecycle != "active": - continue - for mapping in entry.mappings: - if mapping.endpoint != endpoint or mapping.channel != "problem_details": - continue - assert mapping.http_status is not None - pairs.add((entry.code, mapping.http_status)) - return frozenset(pairs) - - def _assert_endpoint_bijection(*, endpoint: str, contribution: DoorContribution) -> None: served = frozenset(_served_status_codes(contribution, endpoint=endpoint)) - registered = _registered_status_codes(endpoint) + registered = registered_status_codes(endpoint) unregistered = sorted(served - registered) unserved = sorted(registered - served) assert not unregistered and not unserved, ( diff --git a/tests/platform/test_registry_endpoint.py b/tests/platform/test_registry_endpoint.py index f6e27e8d..1403afcc 100644 --- a/tests/platform/test_registry_endpoint.py +++ b/tests/platform/test_registry_endpoint.py @@ -41,6 +41,10 @@ def _client(**overrides: object) -> TestClient: async def route_a() -> None: raise _Unsupported() + @router.post("/api/t/a") + async def route_a_posted() -> None: + raise _Unsupported() + @router.get("/api/t/a/admitted") async def route_admitted() -> None: return None @@ -64,7 +68,12 @@ async def route_unbound() -> None: middleware=(_admit,), registry_endpoints=overrides.get( "registry_endpoints", - {"/api/t/a": "t.a", "/api/t/a/admitted": "t.a", "/api/t/b/{item}": "t.b"}, + { + ("GET", "/api/t/a"): "t.a", + ("POST", "/api/t/a"): "t.b", + ("GET", "/api/t/a/admitted"): "t.a", + ("GET", "/api/t/b/{item}"): "t.b", + }, ), ) return TestClient(create_app([surface]), raise_server_exceptions=False) @@ -86,9 +95,13 @@ def test_a_refusal_renders_at_the_status_of_its_routes_endpoint(path: str, statu assert response.status_code == status, response.text +def test_two_methods_on_one_path_serve_their_own_endpoints() -> None: + assert _client().post("/api/t/a").status_code == 501 + + def test_a_registry_endpoint_naming_no_route_is_refused() -> None: with pytest.raises(SurfaceConfigurationError): - _client(registry_endpoints={"/api/t/missing": "t.a"}) + _client(registry_endpoints={("GET", "/api/t/missing"): "t.a"}) @pytest.mark.parametrize( diff --git a/tests/presentation_api/test_capability_version.py b/tests/presentation_api/test_capability_version.py index 1526f347..68c51471 100644 --- a/tests/presentation_api/test_capability_version.py +++ b/tests/presentation_api/test_capability_version.py @@ -119,8 +119,8 @@ def test_frame_unavailable_outcome_change_moves_the_token(release: CatalogueRele baseline = version.compute_presentation_capability_version(release=release) mutated_outcome = dict(version._FRAME_UNAVAILABLE_OUTCOME) reason = next(iter(sorted(mutated_outcome))) - code, status, retryable = mutated_outcome[reason] - mutated_outcome[reason] = (code, status, not retryable) + code, retryable = mutated_outcome[reason] + mutated_outcome[reason] = (code, not retryable) perturbed = version.compute_presentation_capability_version( release=release, frame_unavailable_outcome=mutated_outcome @@ -129,6 +129,21 @@ def test_frame_unavailable_outcome_change_moves_the_token(release: CatalogueRele assert perturbed != baseline +def test_a_registry_status_change_on_any_endpoint_moves_the_token(release: CatalogueRelease): + """A pin bump that changes what the render routes serve moves the token.""" + baseline = version.compute_presentation_capability_version(release=release) + statuses = { + endpoint: dict(codes) for endpoint, codes in version.presentation_statuses().items() + } + statuses["presentation.render"]["frame_unavailable"] = 410 + + perturbed = version.compute_presentation_capability_version( + release=release, registry_statuses=statuses + ) + + assert perturbed != baseline + + def test_presentation_capability_version_is_pres_prefixed_and_deterministic( release: CatalogueRelease, ): diff --git a/tests/presentation_api/test_frames_door.py b/tests/presentation_api/test_frames_door.py index b53a0ded..71166468 100644 --- a/tests/presentation_api/test_frames_door.py +++ b/tests/presentation_api/test_frames_door.py @@ -491,7 +491,7 @@ def test_a_bare_id_the_runner_takes_no_spec_root_of_is_request_invalid( 503, "service_unavailable", True, - None, + "pending", id="building", ), pytest.param( @@ -503,7 +503,7 @@ def test_a_bare_id_the_runner_takes_no_spec_root_of_is_request_invalid( id="not-registered", ), pytest.param( - MintBackpressure(), 503, "service_unavailable", True, None, id="mint-backpressure" + MintBackpressure(), 503, "service_unavailable", True, "pending", id="mint-backpressure" ), pytest.param( IdentityQuarantined(subject_id=BARE_SPEC_ID, id_kind="spec"), @@ -538,7 +538,7 @@ def test_a_bare_spec_id_that_cannot_be_served_answers_by_cause( status: int, code: str, retryable: bool, - cause: str | None, + cause: str, ): newest_specs.answer = answer # ty: ignore[invalid-assignment] @@ -547,7 +547,7 @@ def test_a_bare_spec_id_that_cannot_be_served_answers_by_cause( assert resp.status_code == status, resp.text body = resp.json() assert (body["code"], body["retryable"]) == (code, retryable) - assert (body.get("cause") or {}).get("code") == cause + assert body["cause"]["code"] == cause def test_bare_reads_at_their_limit_refuse_and_leave_pair_reads_their_permits( diff --git a/tests/presentation_api/test_refusal_registry.py b/tests/presentation_api/test_refusal_registry.py index b2cae46b..d8a6810d 100644 --- a/tests/presentation_api/test_refusal_registry.py +++ b/tests/presentation_api/test_refusal_registry.py @@ -1,56 +1,94 @@ -"""Every Presentation route that serves a code registry endpoint binds it, and every exemplar goes -out at the status the registry gives its code on the endpoint whose routes raise it: a render -door exemplar on `presentation.render`, a shares door exemplar on `presentation.shares`, and every -other exemplar on `presentation.write`. - -The registry maps no endpoint onto Presentation's read routes. They share the write routes' -classifiers, so they serve the write routes' statuses. +"""Every Presentation route but the meta routes binds the code registry endpoint it serves, and +every exemplar and `FrameUnavailable` reason a door's routes raise goes out, through the surface's +classifier with the route's endpoint bound, at the one status the registry gives its code there. """ from __future__ import annotations -from dfo_spec_kit.registry import REGISTRY +import logging +from collections import Counter + +import pytest -from assembler.errors import FrameUnavailable +from assembler.errors import ( + BindingInvalid, + ConstraintViolation, + FrameUnavailable, + ReleaseMismatch, + ResourceExhausted, + SchemaInvalid, + WriteRefused, +) from assembler.fakes import FakeReleaseRegistry +from assembler.pipeline import ComponentHoldsNoFrame +from assembler.ports import FrameUnavailableReason +from datasets_api.allowance import AllowanceExhausted +from datasets_api.door_executor import build_door_executor +from datasets_api.live_refresh import RefreshNotPermitted from dfo_platform.codes import bound_registry_endpoint +from dfo_platform.doors import DoorContribution from presentation_api.app import presentation_surface -from presentation_api.doors.render import RENDER_EXCEPTION_EXEMPLARS -from presentation_api.doors.shares import SHARES_EXCEPTION_EXEMPLARS -from presentation_api.refusals import _FRAME_UNAVAILABLE_OUTCOME - -_WRITE = "presentation.write" -_RENDER = "presentation.render" -_SHARES = "presentation.shares" - -# Codes of the write routes alone. No render route raises them, and every other code a non-share -# exemplar classifies to must be mapped on `presentation.render`, whose routes restate it. -_WRITE_ONLY_CODES = frozenset({"write_refused", "allowance_exhausted"}) +from presentation_api.codes import ( + _UNCHANGED_RETRY_MODES, + CODE_RETRY_MODE, + RENDER_ENDPOINT, + served_status, +) +from presentation_api.doors.components import build_components_door +from presentation_api.doors.frames import build_frames_door +from presentation_api.doors.render import RenderTargetMismatch +from presentation_api.doors.shares import build_shares_door +from presentation_api.refusals import _FRAME_UNAVAILABLE_OUTCOME, frame_unavailable_spec +from tests.code_registry import problem_statuses, registered_endpoints, unbound_routes -# Routes that serve no registry endpoint. The component read shares the revise route's path, -# so it is bound with it. +# Routes that serve no registry endpoint: they refuse only with platform codes. _UNBOUND_PATHS = frozenset( { "/api/presentation/healthz", "/api/presentation/capabilities", "/api/presentation/release", - "/api/presentation/frames/{spec_id}", - "/api/presentation/frames/{spec_id}/{content_version}", - "/api/presentation/components/{component_id}/revisions/{revision}/data", } ) - -def _statuses(code: str, endpoint: str) -> set[int]: - """The statuses the registry gives `code`'s problem details on `endpoint`.""" - entry = REGISTRY[code] if code in REGISTRY else None # noqa: SIM401 -- CodeRegistry has no .get() - return { - mapping.http_status - for mapping in (entry.mappings if entry is not None else ()) - if mapping.channel == "problem_details" - and mapping.http_status is not None - and mapping.endpoint == endpoint +# The endpoints whose routes load or pin a frame. +_FRAME_ENDPOINTS = frozenset( + { + "presentation.write", + "presentation.component_data", + "presentation.frames", + "presentation.render", + "presentation.shares", } +) + +_RENDER_ONLY = (RefreshNotPermitted, RenderTargetMismatch, ComponentHoldsNoFrame) +_WRITES_AND_DRAWS = ( + AllowanceExhausted, + WriteRefused, + SchemaInvalid, + ConstraintViolation, + ResourceExhausted, +) + +# What each endpoint's routes do not raise, of the exemplars of the door that serves them. +_NOT_RAISED: dict[str, tuple[type[BaseException], ...]] = { + "presentation.write": _RENDER_ONLY, + "presentation.component_read": (*_RENDER_ONLY, *_WRITES_AND_DRAWS, BindingInvalid), + "presentation.component_data": (*_RENDER_ONLY, *_WRITES_AND_DRAWS, ReleaseMismatch), + "presentation.render": (AllowanceExhausted, WriteRefused), + "presentation.frames": (), + "presentation.shares": (RefreshNotPermitted,), +} + + +@pytest.fixture +def doors(release_registry: FakeReleaseRegistry) -> tuple[DoorContribution, ...]: + reads = build_door_executor(1, "test-frames-bare", surface="presentation") + return ( + build_components_door(release_registry=release_registry, bare_reads=reads), + build_frames_door(bare_reads=reads), + build_shares_door(release_registry=release_registry), + ) def test_every_presentation_route_binds_the_registry_endpoint_it_serves( @@ -59,58 +97,75 @@ def test_every_presentation_route_binds_the_registry_endpoint_it_serves( """The surface binds every Presentation endpoint the registry maps, each to a route it serves, and leaves only the routes in `_UNBOUND_PATHS` unbound.""" surface = presentation_surface(release_registry=release_registry) - registered = { - mapping.endpoint - for entry in REGISTRY.entries - for mapping in entry.mappings - if mapping.endpoint.startswith("presentation.") - } - routed = {getattr(route, "path", None) for route in surface.router.routes} - assert set(surface.registry_endpoints.values()) == registered - assert routed - set(surface.registry_endpoints) == _UNBOUND_PATHS + assert set(surface.registry_endpoints.values()) == registered_endpoints("presentation.") + assert {path for _, path in unbound_routes(surface)} == _UNBOUND_PATHS -def test_every_presentation_exemplar_goes_out_at_its_registered_status( - release_registry: FakeReleaseRegistry, +def test_each_presentation_endpoint_belongs_to_one_door(doors: tuple[DoorContribution, ...]): + """`classify_by_door` tries the door that owns the bound endpoint first, so a shared endpoint + would hand one door's routes to the other.""" + owners = Counter( + endpoint for door in doors for endpoint in set(door.registry_endpoints.values()) + ) + assert [endpoint for endpoint, count in owners.items() if count > 1] == [] + + +def _raised_on(endpoint: str, exemplar: BaseException) -> bool: + """Whether a route of `endpoint` raises `exemplar`: a door's exemplars cover every route it + serves.""" + return not isinstance(exemplar, _NOT_RAISED[endpoint]) + + +def test_every_presentation_refusal_goes_out_at_its_registered_status( + release_registry: FakeReleaseRegistry, doors: tuple[DoorContribution, ...] ): - """Each exemplar, classified with its routes' endpoint bound, goes out at the one status the - registry gives its code there, and its code is mapped on that endpoint. So does every - `FrameUnavailable` reason, on the write and share routes.""" + """A code a route raises and the registry leaves off its endpoint fails here, and on a render + route `retryable` holds only for a code the same request may pass later.""" surface = presentation_surface(release_registry=release_registry) - render_exemplars = {id(exemplar) for exemplar in RENDER_EXCEPTION_EXEMPLARS} - share_exemplars = {id(exemplar) for exemplar in SHARES_EXCEPTION_EXEMPLARS} - reasons = [ + reasons = tuple( FrameUnavailable(reason=reason, detail="exemplar") for reason in _FRAME_UNAVAILABLE_OUTCOME - ] - bound: list[tuple[str, BaseException]] = [ - *((_WRITE, exemplar) for exemplar in reasons), - *((_SHARES, exemplar) for exemplar in reasons), - ] - for exemplar in surface.exception_exemplars: - if id(exemplar) in render_exemplars: - bound.append((_RENDER, exemplar)) - elif id(exemplar) in share_exemplars: - bound.append((_SHARES, exemplar)) - else: - bound.append((_WRITE, exemplar)) + ) disagreements: set[tuple[str, str, str, int]] = set() - unmapped: set[tuple[str, str, str]] = set() - unmapped_on_render: set[str] = set() - checked: set[str] = set() - for endpoint, exemplar in bound: - with bound_registry_endpoint(endpoint): - spec = surface.classify(exemplar) - assert spec is not None, repr(exemplar) - checked.add(endpoint) - statuses = _statuses(spec.code, endpoint) - if not statuses: - unmapped.add((type(exemplar).__qualname__, endpoint, spec.code)) - elif statuses != {spec.status}: - disagreements.add((type(exemplar).__qualname__, endpoint, spec.code, spec.status)) - if endpoint == _WRITE and not _statuses(spec.code, _RENDER): - unmapped_on_render.add(spec.code) - assert checked == {_WRITE, _RENDER, _SHARES} + swept: set[str] = set() + for door in doors: + for endpoint in set(door.registry_endpoints.values()): + frame_refusals = reasons if endpoint in _FRAME_ENDPOINTS else () + for exemplar in (*door.exception_exemplars, *frame_refusals): + if not _raised_on(endpoint, exemplar): + continue + with bound_registry_endpoint(endpoint): + spec = surface.classify(exemplar) + assert spec is not None, repr(exemplar) + swept.add(endpoint) + if problem_statuses(spec.code, endpoint) != {spec.status}: + disagreements.add( + (type(exemplar).__qualname__, endpoint, spec.code, spec.status) + ) + if endpoint == RENDER_ENDPOINT and spec.retryable: + assert CODE_RETRY_MODE[spec.code] in _UNCHANGED_RETRY_MODES, repr(exemplar) + assert swept == registered_endpoints("presentation.") assert sorted(disagreements) == [] - assert sorted(unmapped) == [] - assert unmapped_on_render == _WRITE_ONLY_CODES + + +@pytest.mark.parametrize( + ("reason", "logged"), + [("io", True), ("recipe_unreadable", True), ("pending", False), ("expired", False)], +) +def test_a_frame_refusal_that_names_a_server_fault_logs_an_error( + caplog: pytest.LogCaptureFixture, reason: FrameUnavailableReason, logged: bool +): + with bound_registry_endpoint("presentation.shares"), caplog.at_level(logging.ERROR): + frame_unavailable_spec(FrameUnavailable(reason=reason, detail="exemplar")) + + assert bool(caplog.records) is logged + + +def test_a_code_the_bound_endpoint_leaves_unmapped_keeps_its_refusal_and_logs( + caplog: pytest.LogCaptureFixture, +): + with bound_registry_endpoint(RENDER_ENDPOINT), caplog.at_level(logging.ERROR): + status = served_status("write_refused") + + assert status == 403 + assert "write_refused" in caplog.text diff --git a/tests/presentation_api/test_render_door.py b/tests/presentation_api/test_render_door.py index f4e8f594..0cd41123 100644 --- a/tests/presentation_api/test_render_door.py +++ b/tests/presentation_api/test_render_door.py @@ -818,6 +818,13 @@ def expired(*, frame_ref: FrameRef): False, id="frame-unavailable-revoked", ), + pytest.param( + FrameUnavailable(reason="io", detail="the store did not answer"), + 503, + "service_unavailable", + True, + id="frame-store-io", + ), pytest.param( ReleaseMismatch(catalog_version="2099.1.0", detail="newer"), 409, diff --git a/tests/presentation_api/test_share_data_door.py b/tests/presentation_api/test_share_data_door.py index ff2517ee..12ef618e 100644 --- a/tests/presentation_api/test_share_data_door.py +++ b/tests/presentation_api/test_share_data_door.py @@ -609,10 +609,18 @@ def test_a_slot_the_share_does_not_have_refuses(door: _Door, path: str, status: @pytest.mark.parametrize( ("reason", "status", "code", "retryable"), [ - ("io", 503, "service_unavailable", True), + ("expired", 404, "frame_unavailable", True), ("revoked", 404, "frame_unavailable", False), + ("ops_unsupported", 404, "frame_unavailable", True), + ("missing_rendering", 404, "frame_unavailable", False), + ("digest_mismatch", 404, "frame_unavailable", False), + ("quarantined", 404, "frame_unavailable", False), + ("recipe_pending", 404, "frame_unavailable", True), + ("unknown", 404, "frame_unavailable", False), + ("io", 503, "service_unavailable", True), + ("pending", 503, "service_unavailable", True), + ("recipe_unreadable", 500, "internal_error", False), ], - ids=["read-failed", "frame-gone"], ) def test_a_slot_whose_frame_does_not_load_answers_by_cause( door: _Door, @@ -632,6 +640,7 @@ def unloadable(*, frame_ref: FrameRef) -> ResolvedFrame: body = resp.json() assert (resp.status_code, body["code"], body["retryable"]) == (status, code, retryable) + assert body["cause"]["code"] == reason def test_an_emptied_slot_and_a_notes_slot_answer_not_found(door: _Door): diff --git a/tests/presentation_store/test_frame_store.py b/tests/presentation_store/test_frame_store.py index 8093777d..49d59314 100644 --- a/tests/presentation_store/test_frame_store.py +++ b/tests/presentation_store/test_frame_store.py @@ -15,8 +15,10 @@ from contextvars import copy_context from dataclasses import dataclass, field, replace from datetime import UTC, date, datetime, timedelta, timezone -from typing import Any, cast +from typing import Any, Never, cast +import pyarrow as pa +import pyarrow.parquet as pq import pytest from dfo_frame_grammar import Frame, canonical_frame_bytes @@ -753,10 +755,40 @@ def test_load_refuses_a_canonical_frame_over_the_row_bound_before_fetching_it(): assert [call for call in artifacts.calls if call[0] == "get"] == [] -def test_load_refuses_a_canonical_frame_whose_bytes_do_not_decode(): +def _parquet_bytes() -> bytes: + sink = io.BytesIO() + table = pa.table({"name": [f"row {index}" for index in range(5000)]}) + pq.write_table(table, sink, compression="snappy") + return sink.getvalue() + + +def _footer_zeroed(data: bytes) -> bytes: + footer = int.from_bytes(data[-8:-4], "little") + return data[: -8 - footer] + bytes(footer) + data[-8:] + + +def _pages_flipped(data: bytes) -> bytes: + flipped = bytearray(data) + for index in range(40, 2000, 3): + flipped[index] ^= 0xFF + return bytes(flipped) + + +@pytest.mark.parametrize( + "corrupt", + [ + pytest.param(lambda _: b"not a parquet file", id="not-parquet"), + pytest.param(lambda _: b"", id="empty"), + pytest.param(_footer_zeroed, id="footer-zeroed"), + pytest.param(_pages_flipped, id="pages-flipped"), + pytest.param(lambda data: data[: len(data) // 2], id="body-truncated"), + pytest.param(lambda data: data[:4] + data[len(data) // 2 :], id="tail-kept"), + ], +) +def test_load_refuses_a_canonical_frame_whose_bytes_do_not_decode(corrupt): """Stored bytes that match their digest and do not decode as a frame never load, so the load refuses `missing_rendering`.""" - snapshots, artifacts, frame_ref = _canonical_frame_version(b"not a parquet file") + snapshots, artifacts, frame_ref = _canonical_frame_version(corrupt(_parquet_bytes())) store = _store(snapshots=snapshots, artifacts=artifacts) with pytest.raises(FrameUnavailable) as excinfo: @@ -765,6 +797,49 @@ def test_load_refuses_a_canonical_frame_whose_bytes_do_not_decode(): assert excinfo.value.reason == "missing_rendering" +@pytest.mark.parametrize( + "raised", + [ + pytest.param(MemoryError(), id="python-out-of-memory"), + pytest.param(pa.ArrowMemoryError("exemplar"), id="arrow-out-of-memory"), + pytest.param(pa.ArrowNotImplementedError("codec not built"), id="codec-not-built"), + ], +) +def test_load_refuses_io_for_a_decode_failure_that_says_nothing_about_the_bytes( + monkeypatch: pytest.MonkeyPatch, raised: Exception +): + def fail(_data: bytes) -> Never: + raise raised + + snapshots, artifacts, frame_ref, _ = _canonical_frame_fixture() + monkeypatch.setattr(frame_store, "read_canonical_frame_table", fail) + + with pytest.raises(FrameUnavailable) as excinfo: + _store(snapshots=snapshots, artifacts=artifacts).load(frame_ref=frame_ref) + + assert excinfo.value.reason == "io" + + +@pytest.mark.parametrize( + "data", + [ + pytest.param(b"not json", id="not-json"), + pytest.param(b"\xff\xfe", id="not-utf8"), + pytest.param(b"[" * 100_000, id="nested-past-the-parser"), + pytest.param(b"null", id="null"), + pytest.param(b"[]", id="list"), + pytest.param(b"{}", id="no-columns"), + pytest.param(b'{"columns": 3, "rows": []}', id="columns-not-a-list"), + pytest.param(b'{"columns": ["a", "b"], "rows": [[1]]}', id="short-row"), + ], +) +def test_a_frame1_document_that_is_not_a_frame_refuses_missing_rendering(data: bytes): + with pytest.raises(FrameUnavailable) as excinfo: + frame_store._decode_frame(data, location=_location(digest="d", tier="working"), max_rows=10) + + assert excinfo.value.reason == "missing_rendering" + + def test_load_decodes_a_canonical_frame_at_the_row_bound(): snapshots, artifacts, frame_ref, frame = _canonical_frame_fixture() store = _store(snapshots=snapshots, artifacts=artifacts, frame_max_rows=frame.table.num_rows) diff --git a/tests/skills_api/test_refusal_registry.py b/tests/skills_api/test_refusal_registry.py index 964e2410..90a01c7d 100644 --- a/tests/skills_api/test_refusal_registry.py +++ b/tests/skills_api/test_refusal_registry.py @@ -1,69 +1,40 @@ """Every Skills route that refuses binds the code registry endpoint it serves, and every exemplar -the Skills surface declares goes out at the status the registry gives its code on each Skills -endpoint that maps it. +each door declares goes out at the one status the registry gives its code on that door's endpoint. """ from __future__ import annotations -from dfo_spec_kit.registry import REGISTRY - from dfo_platform.codes import bound_registry_endpoint from skills_api.app import skills_surface +from skills_api.doors.fetch import build_fetch_door +from skills_api.doors.list import build_list_door +from skills_api.doors.resource import build_resource_door +from tests.code_registry import problem_statuses, registered_endpoints, unbound_routes # Routes that serve no registry endpoint: they refuse only with platform codes. _UNBOUND_PATHS = frozenset({"/api/skills/healthz"}) -def _statuses(code: str, endpoint: str) -> set[int]: - """The statuses the registry gives `code`'s problem details on `endpoint`.""" - entry = REGISTRY[code] if code in REGISTRY else None # noqa: SIM401 -- CodeRegistry has no .get() - return { - mapping.http_status - for mapping in (entry.mappings if entry is not None else ()) - if mapping.channel == "problem_details" - and mapping.http_status is not None - and mapping.endpoint == endpoint - } - - def test_every_skills_route_binds_the_registry_endpoint_it_serves(): """The surface binds every Skills endpoint the registry maps, each to a route it serves, and leaves only the routes in `_UNBOUND_PATHS` unbound.""" surface = skills_surface(skills_index=object()) - registered = { - mapping.endpoint - for entry in REGISTRY.entries - for mapping in entry.mappings - if mapping.endpoint.startswith("skills.") - } - routed = {getattr(route, "path", None) for route in surface.router.routes} - assert set(surface.registry_endpoints.values()) == registered - assert routed - set(surface.registry_endpoints) == _UNBOUND_PATHS + assert set(surface.registry_endpoints.values()) == registered_endpoints("skills.") + assert {path for _, path in unbound_routes(surface)} == _UNBOUND_PATHS def test_every_skills_exemplar_goes_out_at_its_registered_status(): - """Each exemplar, classified with each Skills endpoint bound, goes out at the one status the - registry gives its code there. Its code is mapped on at least one Skills endpoint.""" - surface = skills_surface(skills_index=object()) - endpoints = sorted(set(surface.registry_endpoints.values())) - assert endpoints + """Each door's exemplars, classified with the door's own endpoint bound, go out at the one + status the registry gives their code there.""" disagreements: set[tuple[str, str, str, int]] = set() - unmapped: set[str] = set() - for exemplar in surface.exception_exemplars: - mapped = False - for endpoint in endpoints: + for door in (build_list_door(), build_fetch_door(), build_resource_door()): + (endpoint,) = set(door.registry_endpoints.values()) + assert door.exception_exemplars + for exemplar in door.exception_exemplars: with bound_registry_endpoint(endpoint): - spec = surface.classify(exemplar) - if spec is None: - continue - statuses = _statuses(spec.code, endpoint) - if not statuses: - continue - mapped = True - if statuses != {spec.status}: + spec = door.classify(exemplar) + assert spec is not None, repr(exemplar) + if problem_statuses(spec.code, endpoint) != {spec.status}: disagreements.add((type(exemplar).__qualname__, endpoint, spec.code, spec.status)) - if not mapped: - unmapped.add(type(exemplar).__qualname__) assert sorted(disagreements) == [] - assert sorted(unmapped) == [] diff --git a/uv.lock b/uv.lock index f339665e..a23319a6 100644 --- a/uv.lock +++ b/uv.lock @@ -671,8 +671,8 @@ dependencies = [ [package.metadata] requires-dist = [ - { name = "dfo-spec-kit", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3" }, - { name = "dfo-spec-kit-frame-grammar", extras = ["arrow"], git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3" }, + { name = "dfo-spec-kit", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=32c47517128d6c4311c09bd5072203edd859fc48" }, + { name = "dfo-spec-kit-frame-grammar", extras = ["arrow"], git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=32c47517128d6c4311c09bd5072203edd859fc48" }, { name = "pyarrow", specifier = ">=24" }, { name = "pydantic", specifier = ">=2.13" }, { name = "rfc8785", specifier = ">=0.1.4" }, @@ -746,10 +746,10 @@ requires-dist = [ { name = "dfo-data-tools", extras = ["generation"], marker = "extra == 'resolve'", editable = "packages/dfo-data-tools" }, { name = "dfo-data-tools", extras = ["sdmx"], marker = "extra == 'coverage'", editable = "packages/dfo-data-tools" }, { name = "dfo-data-tools", extras = ["sdmx"], marker = "extra == 'formats'", editable = "packages/dfo-data-tools" }, - { name = "dfo-spec-kit-components", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdfo-spec-kit-components&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3" }, - { name = "dfo-spec-kit-descriptions", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdescriptions&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3" }, - { name = "dfo-spec-kit-frame-grammar", extras = ["arrow"], git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3" }, - { name = "dfo-spec-kit-provenance-docs", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fprovenance-docs&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3" }, + { name = "dfo-spec-kit-components", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdfo-spec-kit-components&rev=32c47517128d6c4311c09bd5072203edd859fc48" }, + { name = "dfo-spec-kit-descriptions", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdescriptions&rev=32c47517128d6c4311c09bd5072203edd859fc48" }, + { name = "dfo-spec-kit-frame-grammar", extras = ["arrow"], git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=32c47517128d6c4311c09bd5072203edd859fc48" }, + { name = "dfo-spec-kit-provenance-docs", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fprovenance-docs&rev=32c47517128d6c4311c09bd5072203edd859fc48" }, { name = "fastapi", marker = "extra == 'surface'", specifier = ">=0.138" }, { name = "google-auth", extras = ["requests"], marker = "extra == 'generation'", specifier = ">=2.55" }, { name = "google-auth", extras = ["requests"], marker = "extra == 'sdmx'", specifier = ">=2.55" }, @@ -775,7 +775,7 @@ provides-extras = ["sdmx", "formats", "surface", "resolve", "generation", "cover [[package]] name = "dfo-spec-kit" version = "0.2.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3#2dd2400a36d592bd3ca814aa2eff30bd016433e3" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=32c47517128d6c4311c09bd5072203edd859fc48#32c47517128d6c4311c09bd5072203edd859fc48" } dependencies = [ { name = "rfc8785" }, ] @@ -783,12 +783,12 @@ dependencies = [ [[package]] name = "dfo-spec-kit-client-contract" version = "0.1.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fclient-contract&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3#2dd2400a36d592bd3ca814aa2eff30bd016433e3" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fclient-contract&rev=32c47517128d6c4311c09bd5072203edd859fc48#32c47517128d6c4311c09bd5072203edd859fc48" } [[package]] name = "dfo-spec-kit-components" version = "0.1.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdfo-spec-kit-components&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3#2dd2400a36d592bd3ca814aa2eff30bd016433e3" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdfo-spec-kit-components&rev=32c47517128d6c4311c09bd5072203edd859fc48#32c47517128d6c4311c09bd5072203edd859fc48" } dependencies = [ { name = "dfo-spec-kit-descriptions" }, ] @@ -796,7 +796,7 @@ dependencies = [ [[package]] name = "dfo-spec-kit-descriptions" version = "0.1.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdescriptions&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3#2dd2400a36d592bd3ca814aa2eff30bd016433e3" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fdescriptions&rev=32c47517128d6c4311c09bd5072203edd859fc48#32c47517128d6c4311c09bd5072203edd859fc48" } dependencies = [ { name = "rfc8785" }, ] @@ -804,7 +804,7 @@ dependencies = [ [[package]] name = "dfo-spec-kit-frame-grammar" version = "0.1.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3#2dd2400a36d592bd3ca814aa2eff30bd016433e3" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fframe-grammar&rev=32c47517128d6c4311c09bd5072203edd859fc48#32c47517128d6c4311c09bd5072203edd859fc48" } dependencies = [ { name = "dfo-spec-kit" }, { name = "pyarrow" }, @@ -814,7 +814,7 @@ dependencies = [ [[package]] name = "dfo-spec-kit-provenance-docs" version = "0.1.0" -source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fprovenance-docs&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3#2dd2400a36d592bd3ca814aa2eff30bd016433e3" } +source = { git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fprovenance-docs&rev=32c47517128d6c4311c09bd5072203edd859fc48#32c47517128d6c4311c09bd5072203edd859fc48" } dependencies = [ { name = "markdown-it-py" }, ] @@ -2964,7 +2964,7 @@ requires-dist = [ { name = "dfo-data-tools", extras = ["resolve", "sdmx", "coverage"], marker = "extra == 'graph'", editable = "packages/dfo-data-tools" }, { name = "dfo-data-tools", extras = ["sdmx"], marker = "extra == 'sdmx'", editable = "packages/dfo-data-tools" }, { name = "dfo-data-tools", extras = ["surface", "resolve", "coverage", "sdmx"], marker = "extra == 'engine'", editable = "packages/dfo-data-tools" }, - { name = "dfo-spec-kit", marker = "extra == 'engine'", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3" }, + { name = "dfo-spec-kit", marker = "extra == 'engine'", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fspec-kit&rev=32c47517128d6c4311c09bd5072203edd859fc48" }, { name = "fastapi", marker = "extra == 'api'", specifier = ">=0.138" }, { name = "google-auth", marker = "extra == 'datasets'", specifier = ">=2.34" }, { name = "google-auth", extras = ["requests"], marker = "extra == 'eval'", specifier = ">=2.34" }, @@ -2996,7 +2996,7 @@ lint = [ ] test = [ { name = "dfo-analysis-engine", editable = "packages/dfo-analysis-engine" }, - { name = "dfo-spec-kit-client-contract", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fclient-contract&rev=2dd2400a36d592bd3ca814aa2eff30bd016433e3" }, + { name = "dfo-spec-kit-client-contract", git = "https://github.com/DevelopmentFinanceObservatory/spec-kit.git?subdirectory=python%2Fclient-contract&rev=32c47517128d6c4311c09bd5072203edd859fc48" }, { name = "frictionless", specifier = ">=5.19.0" }, { name = "pytest", specifier = ">=8" }, { name = "pytest-split", specifier = ">=0.10.0" },