From 023aea1acafa4a42461f58ec4086b95eb9c0b48a Mon Sep 17 00:00:00 2001 From: Kingston Date: Sat, 22 Aug 2026 18:15:56 -0700 Subject: [PATCH] refactor(server): make a refusal its own HTTP response `MediaResolutionError` and `SidecarError` subclassed plain `Exception` and carried a status and detail that something else had to convert: two converter functions, and a catch-and-re-raise at all six call sites that did nothing but copy two fields into an `HTTPException`. Subclassing `HTTPException` instead makes the refusal the response. Raised from anywhere under a route, FastAPI renders it with the status and detail the raising module chose, so both converters and every rewrap go. 52 deletions, 30 insertions; 2,708 -> 2,679 code lines. They stay two types rather than collapsing into one shared refusal, because `_media.is_uri_servable` catches media refusals specifically -- a single shared type would let it swallow an unrelated refusal raised nearby. Behaviour is unchanged and checked rather than assumed: the OpenAPI schema dumps byte-identical before and after, and every status assertion in the media, sidecar and pin suites still passes (645 tests). Found by an audit of whether this package earns its lines. Its verdict was mostly no-change -- roughly 1% of the code was signed off as safely deletable, and the file count was explicitly not the problem. Co-Authored-By: Claude Opus 5 --- .../src/hflow_server/_curation.py | 34 ++++++------------- .../hflow-server/src/hflow_server/_media.py | 25 +++++--------- .../hflow-server/src/hflow_server/_sidecar.py | 14 ++++---- .../hflow-server/src/hflow_server/server.py | 9 +++-- 4 files changed, 30 insertions(+), 52 deletions(-) diff --git a/packages/hflow-server/src/hflow_server/_curation.py b/packages/hflow-server/src/hflow_server/_curation.py index 5f049c5f..96d2bab5 100644 --- a/packages/hflow-server/src/hflow_server/_curation.py +++ b/packages/hflow-server/src/hflow_server/_curation.py @@ -173,10 +173,6 @@ def _reject_non_single_select(user_sql: str) -> None: ) -def _sidecar_refusal(error: _sidecar.SidecarError) -> HTTPException: - return HTTPException(status_code=error.status_code, detail=error.detail) - - def _browsable_relations( connection: duckdb.DuckDBPyConnection, ) -> dict[str, CatalogTableKind]: @@ -310,23 +306,17 @@ def create_curation_router(settings: ServerSettings) -> APIRouter: # mutating route -- sufficient for the single-server design. sidecar_write_lock = threading.Lock() + # A SidecarError is already an HTTPException, so these three only bind the + # data root -- nothing catches and re-raises, and a refusal raised inside + # them reaches the client with the status and detail _sidecar chose. def loaded_sidecar_state() -> _sidecar.SidecarState: - try: - return _sidecar.load_sidecar_state(settings.data_root) - except _sidecar.SidecarError as error: - raise _sidecar_refusal(error) from error + return _sidecar.load_sidecar_state(settings.data_root) def stored_sidecar_state(state: _sidecar.SidecarState) -> None: - try: - _sidecar.store_sidecar_state(settings.data_root, state) - except _sidecar.SidecarError as error: - raise _sidecar_refusal(error) from error + _sidecar.store_sidecar_state(settings.data_root, state) def local_data_root_or_refuse() -> Path: - try: - return _sidecar.local_data_root(settings.data_root) - except _sidecar.SidecarError as error: - raise _sidecar_refusal(error) from error + return _sidecar.local_data_root(settings.data_root) @router.post("/curation/preview") def run_curation_preview(request: PreviewRequest) -> CurationPreviewResponse: @@ -420,14 +410,10 @@ def download_manifest(manifest_id: str) -> FileResponse: status_code=404, detail=f"no pinned manifest with id {manifest_id!r}" ) manifest_file = local_data_root_or_refuse() / entry.manifest_path - try: - # The same strict-resolve + containment check media serving uses: - # even a hand-edited registry path can only serve workspace files. - resolved_file = _media.resolve_served_file( - str(manifest_file), data_root=settings.data_root - ) - except _media.MediaResolutionError as error: - raise _media.media_refusal(error) from error + # The same strict-resolve + containment check media serving uses: even + # a hand-edited registry path can only serve workspace files. Its + # refusal is already an HTTPException, so it needs no rewrapping here. + resolved_file = _media.resolve_served_file(str(manifest_file), data_root=settings.data_root) return _media.served_file_response( resolved_file, attachment_filename=Path(entry.manifest_path).name ) diff --git a/packages/hflow-server/src/hflow_server/_media.py b/packages/hflow-server/src/hflow_server/_media.py index 39699389..4756e32d 100644 --- a/packages/hflow-server/src/hflow_server/_media.py +++ b/packages/hflow-server/src/hflow_server/_media.py @@ -68,24 +68,15 @@ } -class MediaResolutionError(Exception): - """One refusal to serve a catalog URI, carrying its HTTP mapping.""" - - def __init__(self, status_code: int, detail: str) -> None: - super().__init__(detail) - self.status_code = status_code - self.detail = detail - - -def media_refusal(error: MediaResolutionError) -> HTTPException: - """The HTTP refusal one unservable URI maps to. - - Lives beside the error it converts (as ``_connections`` and ``_runtime`` - do for theirs), so the two routes that serve catalog bytes -- episode - media and manifest downloads -- share one mapping instead of each copying - the two field reads. +class MediaResolutionError(HTTPException): + """One refusal to serve a catalog URI. + + It IS the HTTP refusal rather than something a caller converts into one: + raised from anywhere under a route, FastAPI renders it with the status and + detail given here. It stays its own type so + :func:`is_uri_servable` can catch media refusals specifically, without also + swallowing an unrelated refusal raised nearby. """ - return HTTPException(status_code=error.status_code, detail=error.detail) def _resolved_local_data_root(data_root: str) -> Path: diff --git a/packages/hflow-server/src/hflow_server/_sidecar.py b/packages/hflow-server/src/hflow_server/_sidecar.py index 19c23e2c..814a06d3 100644 --- a/packages/hflow-server/src/hflow_server/_sidecar.py +++ b/packages/hflow-server/src/hflow_server/_sidecar.py @@ -35,6 +35,8 @@ from dataclasses import dataclass from pathlib import Path +from fastapi import HTTPException + from hflow_server._contract import CheckCoverageEntry, PinnedManifestEntry, SavedQueryEntry from hflow_server._settings import local_data_root_or_none @@ -43,13 +45,13 @@ SIDECAR_FILE_NAME = "state.json" -class SidecarError(Exception): - """One refusal to read or write the sidecar, carrying its HTTP mapping.""" +class SidecarError(HTTPException): + """One refusal to read or write the sidecar. - def __init__(self, status_code: int, detail: str) -> None: - super().__init__(detail) - self.status_code = status_code - self.detail = detail + It IS the HTTP refusal rather than something the router converts into one: + raised from anywhere under a route, FastAPI renders it with the status and + detail given here. + """ @dataclass(frozen=True) diff --git a/packages/hflow-server/src/hflow_server/server.py b/packages/hflow-server/src/hflow_server/server.py index 430e735f..426f1019 100644 --- a/packages/hflow-server/src/hflow_server/server.py +++ b/packages/hflow-server/src/hflow_server/server.py @@ -374,11 +374,10 @@ def _catalog_marker_readable(workspace: Workspace) -> bool: def _served_file_response_or_refuse(uri: str, data_root: str) -> FileResponse: - try: - resolved_file = _media.resolve_served_file(uri, data_root=data_root) - except _media.MediaResolutionError as error: - raise _media.media_refusal(error) from error - return _media.served_file_response(resolved_file) + """Bytes for one catalog URI. An unservable URI raises its own refusal + (a ``MediaResolutionError`` IS an ``HTTPException``), so there is nothing + to catch and convert here.""" + return _media.served_file_response(_media.resolve_served_file(uri, data_root=data_root)) def _assets_directory(settings: ServerSettings) -> Path | None: