From 46d57a2a1fa65f923fad471d4b619f817d860986 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 12:23:11 +0100 Subject: [PATCH 01/18] This adds validation of return values to actions. Action outputs are serialized using a pydantic model. We now create (and validate) this model instance in the action thread, rather than in the response handler returning it to the user. This means that validation errors will now be caught and logged, and there won't be loads of ASGI/routing/FastAPI stuff in the stack trace. It does not yet fix the problem of un-JSONable types being returned, because `pydantic` will allow `Any` to be wrapped in a `RootModel` which validates fine, and fails at serialisation time. --- src/labthings_fastapi/actions.py | 46 ++++++++++++++++++++++------- src/labthings_fastapi/exceptions.py | 17 +++++++++++ tests/test_actions.py | 44 +++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 11 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 888acfef..51ec5cce 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -40,8 +40,7 @@ from weakref import WeakSet import weakref from fastapi import APIRouter, FastAPI, HTTPException, Request, Body, BackgroundTasks -from pydantic import BaseModel, create_model - +from pydantic import BaseModel, create_model, ValidationError from .middleware.url_for import URLFor from .base_descriptor import ( @@ -50,10 +49,15 @@ DescriptorInfoCollection, ) from .logs import add_thing_log_destination -from .utilities import model_to_dict, wrap_plain_types_in_rootmodel +from .utilities import ( + LabThingsRootModelWrapper, + model_to_dict, + wrap_plain_types_in_rootmodel, +) from .invocations import InvocationModel, InvocationStatus from .exceptions import ( GlobalLockBusyError, + InvalidReturnValue, InvocationCancelledError, InvocationError, NotConnectedToServerError, @@ -142,7 +146,7 @@ def __init__( # Private state properties self._status_lock = Lock() # This Lock protects properties below self._status: InvocationStatus = InvocationStatus.PENDING # Task status - self._return_value: Optional[Any] = None # Return value + self._output_model_instance: Optional[BaseModel] = None # Return value self._request_time: datetime.datetime = datetime.datetime.now() self._start_time: Optional[datetime.datetime] = None # Task start time self._end_time: Optional[datetime.datetime] = None # Task end time @@ -158,7 +162,18 @@ def id(self) -> uuid.UUID: def output(self) -> Any: """Return value of the Action. If the Action is still running, returns None.""" with self._status_lock: - return self._return_value + if self._output_model_instance is None: + return None + if isinstance(self._output_model_instance, LabThingsRootModelWrapper): + return self._output_model_instance.model_dump() + else: + return self._output_model_instance + + @property + def output_model_instance(self) -> BaseModel | None: + """Return value of the Action, as a model, or None.""" + with self._status_lock: + return self._output_model_instance @property def log(self) -> list[logging.LogRecord]: @@ -242,7 +257,7 @@ def response(self) -> InvocationModel: timeCompleted=self._end_time, timeRequested=self._request_time, input=self.input, - output=self.output, + output=self.output_model_instance, links=links, log=self.log, ) @@ -273,6 +288,8 @@ def run(self) -> None: See `.Invocation.status` for status values. :raises RuntimeError: if there is no Thing associated with the invocation. + :raises InvalidReturnValue: if the action returns a value that can't + be validated by its output model. """ # self.action evaluates to an ActionDescriptor. This confuses mypy, # which thinks we are calling ActionDescriptor.__get__. @@ -303,11 +320,18 @@ def run(self) -> None: # Actually run the action ret = action.func(thing, **kwargs, **self.dependencies) - with self._status_lock: - self._return_value = ret - self._status = InvocationStatus.COMPLETED - action.emit_changed_event(self.thing, self._status.value) + try: + output_model_instance = action.output_model.model_validate(ret) + except ValidationError as e: + msg = f"Could not serialise the return value from '{action.name}'. " + msg += f"The output model was '{action.output_model}' and the " + msg += f"return value was '{ret}'." + raise InvalidReturnValue(msg) from e + with self._status_lock: + self._output_model_instance = output_model_instance + self._status = InvocationStatus.COMPLETED + action.emit_changed_event(self.thing, self._status.value) except InvocationCancelledError: logger.info(f"Invocation {self.id} was cancelled.") with self._status_lock: @@ -536,7 +560,7 @@ def action_invocation_output(id: uuid.UUID) -> Any: ): # TODO: honour "accept" header return invocation.output.response() - return invocation.output + return invocation.output_model_instance @router.delete( ACTION_INVOCATIONS_PATH + "/{id}", diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 472d5269..bab67228 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -254,6 +254,23 @@ class NoInvocationContextError(RuntimeError): """ +class InvalidReturnValue(RuntimeError): + r"""The return value from a method cannot be serialised by LabThings. + + This error is raised when an action returns a value that can't be serialised. + This usually means that either it doesn't match the declared return type of + the function, or the declared return type permits un-serialisable values. + + If an action's return type is missing or `Any`\ , it's possible to return a + value that can't be serialised, which will cause this error. + + The solution is usually to ensure that the return type of your action is + either a simple type that can be serialised to JSON, or a Pydantic model. + You should also check that the function's return value matches the declared + type, ideally by regularly running a type checker like `mypy` on your code. + """ + + class LogConfigurationError(RuntimeError): """There is a problem with logging configuration. diff --git a/tests/test_actions.py b/tests/test_actions.py index fbf892ba..eb720d85 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -1,5 +1,7 @@ +from typing import Any import uuid from fastapi.testclient import TestClient +from labthings_fastapi.exceptions import ServerActionError from pydantic import BaseModel import pytest import functools @@ -333,3 +335,45 @@ def long_docstring(self) -> None: assert actions["long_docstring"].description.startswith( "It has multiple paragraphs." ) + + +def test_invalid_return_values(): + """Test the errors raised when an action's return value can't be serialised.""" + + class Unjsonable: + pass + + class NaughtyThing(lt.Thing): + @lt.action + def make_random_int(self) -> int: + """An action that should return an integer, but returns a float.""" + return 4.2 + + @lt.action + def make_unjsonable_any(self) -> Any: + """A vaguely-typed action that won't serialise.""" + return Unjsonable() + + server = lt.ThingServer.from_things({"naughty": NaughtyThing}) + with server.test_client() as client: + tc = lt.ThingClient.from_url("/naughty/", client=client) + + # If a return type doesn't match the type hint, we get + with pytest.raises( + ServerActionError, + match=( + r"\[InvalidReturnValue\]: Could not serialise the return value from " + r"'make_random_int'." + ), + ): + tc.make_random_int() + + # If a return type is not JSONable + with pytest.raises( + ServerActionError, + match=( + r"\[InvalidReturnValue\]: Could not serialise the return value from " + r"'make_unjsonable_any'." + ), + ): + tc.make_unjsonable_any() From 08884e0f128d62ea7c290a0ac2090d84934d9558 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 12:32:38 +0100 Subject: [PATCH 02/18] Improve error message and tidy logging The stack trace is not useful when an action returns an invalid value, as it only tells us about LabThings code. The error is still logged, but there's no stack trace. --- src/labthings_fastapi/actions.py | 8 ++++---- tests/test_actions.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 51ec5cce..9c781a79 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -323,9 +323,9 @@ def run(self) -> None: try: output_model_instance = action.output_model.model_validate(ret) except ValidationError as e: - msg = f"Could not serialise the return value from '{action.name}'. " - msg += f"The output model was '{action.output_model}' and the " - msg += f"return value was '{ret}'." + msg = f"The return value from '{self.thing.name}.{action.name}' " + msg += "failed to validate against its output model " + msg += f"'{action.output_model}'. The return value was '{ret}'." raise InvalidReturnValue(msg) from e with self._status_lock: @@ -339,7 +339,7 @@ def run(self) -> None: action.emit_changed_event(self.thing, self._status.value) except Exception as e: # skipcq: PYL-W0703 # First log - if isinstance(e, InvocationError): + if isinstance(e, (InvocationError, InvalidReturnValue)): # Log without traceback for anticipated errors logger.error(e) elif ( diff --git a/tests/test_actions.py b/tests/test_actions.py index eb720d85..6d7a261f 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -362,8 +362,8 @@ def make_unjsonable_any(self) -> Any: with pytest.raises( ServerActionError, match=( - r"\[InvalidReturnValue\]: Could not serialise the return value from " - r"'make_random_int'." + r"The return value from 'naughty.make_random_int' failed to validate " + r"against its output model." ), ): tc.make_random_int() From 7a0a6a531232a9d404ee44f5f26a3d569ad7ce76 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 14:02:12 +0100 Subject: [PATCH 03/18] Properly handle serialization errors This commit: * Adds an error handler for PydanticSerializationError * Fixes a race condition in invocation responses that could result in an output being present before the response marks the invocation as complete * Handles HTTP errors when polling actions in ThingClient * Ensures that invocation models attach useful debug info to serialization errors (they add the invocation ID and action path). --- src/labthings_fastapi/actions.py | 34 +++++++++++++----------- src/labthings_fastapi/client/__init__.py | 17 +++++++++--- src/labthings_fastapi/invocations.py | 32 +++++++++++++++++++++- src/labthings_fastapi/server/__init__.py | 14 ++++++++++ tests/test_actions.py | 13 +++------ 5 files changed, 81 insertions(+), 29 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 9c781a79..4cb0bcf6 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -22,7 +22,7 @@ from collections import deque from functools import partial, wraps import inspect -from threading import Thread, Lock +from threading import Thread, Lock, RLock import uuid from typing import ( TYPE_CHECKING, @@ -144,7 +144,7 @@ def __init__( self.expiry_time: Optional[datetime.datetime] = None # Private state properties - self._status_lock = Lock() # This Lock protects properties below + self._status_lock = RLock() # This Lock protects properties below self._status: InvocationStatus = InvocationStatus.PENDING # Task status self._output_model_instance: Optional[BaseModel] = None # Return value self._request_time: datetime.datetime = datetime.datetime.now() @@ -248,19 +248,20 @@ def response(self) -> InvocationModel: ] # The line below confuses MyPy because self.action **evaluates to** a Descriptor # object (i.e. we don't call __get__ on the descriptor). - return self.action.invocation_model( # type: ignore[attr-defined] - status=self.status, - id=self.id, - action=self.thing.path + self.action.name, # type: ignore[attr-defined] - href=URLFor("action_invocation", id=self.id), - timeStarted=self._start_time, - timeCompleted=self._end_time, - timeRequested=self._request_time, - input=self.input, - output=self.output_model_instance, - links=links, - log=self.log, - ) + with self._status_lock: + return self.action.invocation_model( # type: ignore[attr-defined] + status=self.status, + id=self.id, + action=self.thing.path + self.action.name, # type: ignore[attr-defined] + href=URLFor("action_invocation", id=self.id), + timeStarted=self._start_time, + timeCompleted=self._end_time, + timeRequested=self._request_time, + input=self.input, + output=self.output_model_instance, + links=links, + log=self.log, + ) def run(self) -> None: """Run the action and track progress. @@ -323,6 +324,9 @@ def run(self) -> None: try: output_model_instance = action.output_model.model_validate(ret) except ValidationError as e: + # Generate a helpful error message. This will be handled below, + # where it will cause the action to be marked as failed, and the + # error will end up in the log. msg = f"The return value from '{self.thing.name}.{action.name}' " msg += "failed to validate against its output model " msg += f"'{action.output_model}'. The return value was '{ret}'." diff --git a/src/labthings_fastapi/client/__init__.py b/src/labthings_fastapi/client/__init__.py index a2ddc247..566852c5 100644 --- a/src/labthings_fastapi/client/__init__.py +++ b/src/labthings_fastapi/client/__init__.py @@ -101,15 +101,24 @@ def poll_invocation( :param first_interval: sets how long we wait before the first polling request. Often, it makes sense for this to be a short interval, in case the action fails (or returns) immediately. - + :raises ServerActionError: if an HTTP error is found during polling. :return: the completed invocation as a dictionary. """ first_time = True while invocation["status"] in ACTION_RUNNING_KEYWORDS: time.sleep(first_interval if first_time else interval) - r = client.get(invocation_href(invocation)) - r.raise_for_status() - invocation = r.json() + response = client.get(invocation_href(invocation)) + if response.is_error: + try: + message = response.json()["detail"] + except KeyError: + message = response.text + raise ServerActionError( + f"The server returned error {response.status_code} while polling " + f"action '{invocation['action']}' with id '{invocation['id']}'. " + f"The error message was:\n{message}." + ) + invocation = response.json() first_time = False return invocation diff --git a/src/labthings_fastapi/invocations.py b/src/labthings_fastapi/invocations.py index 403d1dd5..e010c5d5 100644 --- a/src/labthings_fastapi/invocations.py +++ b/src/labthings_fastapi/invocations.py @@ -11,7 +11,14 @@ from typing import Optional, Any, Sequence, TypeVar, Generic import uuid -from pydantic import BaseModel, ConfigDict, model_validator +from pydantic import ( + BaseModel, + ConfigDict, + model_validator, + model_serializer, + SerializerFunctionWrapHandler, +) +from pydantic_core import PydanticSerializationError from labthings_fastapi.middleware.url_for import URLFor @@ -105,6 +112,29 @@ class GenericInvocationModel(BaseModel, Generic[InputT, OutputT]): log: Sequence[LogRecordModel] links: Links = None + @model_serializer(mode="wrap") + def serialize_model( + self, handler: SerializerFunctionWrapHandler + ) -> dict[str, object]: + """Give a more helpful error if the class fails to serialize. + + :param handler: The Pydantic serializer. + :raises PydanticSerializationError: if the model fails to serialize. This + is wrapped to add the action and invocation ID. + :return: the serialized model, as a dictionary. + """ + try: + return handler(self) + except PydanticSerializationError as e: + extra = "" + if self.output is not None: + extra = "This is often caused by an invalid return value. " + raise PydanticSerializationError( + f"Could not serialise invocation '{self.id}' of '{self.action}' " + f"({self.status.value}). {extra}" + f"Error: '{e}'" + ) from e + InvocationModel = GenericInvocationModel[Any, Any] """A model to serialise `.Invocation` objects when they are polled over HTTP.""" diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 35fa41e4..3771d7bd 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -9,6 +9,7 @@ import warnings from fastapi.testclient import TestClient from pydantic import ValidationError +from pydantic_core import PydanticSerializationError from typing import Any, AsyncGenerator, Optional, TypeVar, overload from fastapi.responses import JSONResponse from typing_extensions import Self @@ -50,6 +51,9 @@ ThingSubclass = TypeVar("ThingSubclass", bound=Thing) +LOGGER = logging.getLogger(__name__) + + class ThingServer: """Use FastAPI to serve `~lt.Thing` instances. @@ -248,6 +252,16 @@ async def global_lock_exception_handler( content={"detail": repr(exc)}, ) + @self.app.exception_handler(PydanticSerializationError) + async def serialization_error_handler( + request: Request, exc: PydanticSerializationError + ) -> JSONResponse: + LOGGER.error( + f"Couldn't serialize response to {request.url} because of error: \n" + f"{exc}" + ) + return JSONResponse(status_code=500, content={"detail": str(exc)}) + @property def debug(self) -> bool: """Whether the server is in debug mode.""" diff --git a/tests/test_actions.py b/tests/test_actions.py index 6d7a261f..9783a5b1 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -340,9 +340,6 @@ def long_docstring(self) -> None: def test_invalid_return_values(): """Test the errors raised when an action's return value can't be serialised.""" - class Unjsonable: - pass - class NaughtyThing(lt.Thing): @lt.action def make_random_int(self) -> int: @@ -352,7 +349,7 @@ def make_random_int(self) -> int: @lt.action def make_unjsonable_any(self) -> Any: """A vaguely-typed action that won't serialise.""" - return Unjsonable() + return object() server = lt.ThingServer.from_things({"naughty": NaughtyThing}) with server.test_client() as client: @@ -371,9 +368,7 @@ def make_unjsonable_any(self) -> Any: # If a return type is not JSONable with pytest.raises( ServerActionError, - match=( - r"\[InvalidReturnValue\]: Could not serialise the return value from " - r"'make_unjsonable_any'." - ), - ): + match="Could not serialise invocation", + ) as excinfo: tc.make_unjsonable_any() + assert "make_unjsonable_any" in str(excinfo) From cf60cd45a00de7a1bed917e1915ca8ddd349432b Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 7 May 2026 09:29:00 +0100 Subject: [PATCH 04/18] Accept FailedToInvokeActionError in tests for bad action output. Depending on whether the action fails before or after the initial response is sent to the client, we get a different exception (with the same message). This commit will accept either kind of failure so long as it includes the same message. This should prevent intermittent test failures: there's nothing to synchronise the action thread and the HTTP response, and it's not feasible to add that now. --- tests/test_actions.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_actions.py b/tests/test_actions.py index 9783a5b1..16425014 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -1,7 +1,7 @@ from typing import Any import uuid from fastapi.testclient import TestClient -from labthings_fastapi.exceptions import ServerActionError +from labthings_fastapi.exceptions import FailedToInvokeActionError, ServerActionError from pydantic import BaseModel import pytest import functools @@ -357,7 +357,7 @@ def make_unjsonable_any(self) -> Any: # If a return type doesn't match the type hint, we get with pytest.raises( - ServerActionError, + (ServerActionError, FailedToInvokeActionError), match=( r"The return value from 'naughty.make_random_int' failed to validate " r"against its output model." @@ -367,7 +367,7 @@ def make_unjsonable_any(self) -> Any: # If a return type is not JSONable with pytest.raises( - ServerActionError, + (ServerActionError, FailedToInvokeActionError), match="Could not serialise invocation", ) as excinfo: tc.make_unjsonable_any() From 3d3be8f5522ea69d4de67587f0591ab49d8cb247 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 11 May 2026 20:59:50 +0100 Subject: [PATCH 05/18] Consolidate root model wrapping code in RootModelWrapper This commit moves `wrap_plain_types_in_rootmodel` into the `RootModelWrapper` class as a class method, and adds better error handling. This now returns class and attribute references, or file and line number in the case of functions. Currently these errors are raised if unserializable types are declared. A future commit will raise similar errors at runtime if unserialisable values are found. --- src/labthings_fastapi/actions.py | 33 ++-- src/labthings_fastapi/exceptions.py | 55 ++++++- src/labthings_fastapi/properties.py | 23 ++- src/labthings_fastapi/utilities/__init__.py | 164 ++++++++++++++------ 4 files changed, 205 insertions(+), 70 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 4cb0bcf6..41080db4 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -50,17 +50,17 @@ ) from .logs import add_thing_log_destination from .utilities import ( - LabThingsRootModelWrapper, + RootModelWrapper, model_to_dict, - wrap_plain_types_in_rootmodel, ) from .invocations import InvocationModel, InvocationStatus from .exceptions import ( GlobalLockBusyError, - InvalidReturnValue, + InvalidReturnValueError, InvocationCancelledError, InvocationError, NotConnectedToServerError, + UnserializableTypeError, ) from . import invocation_contexts from .utilities.introspection import ( @@ -162,12 +162,7 @@ def id(self) -> uuid.UUID: def output(self) -> Any: """Return value of the Action. If the Action is still running, returns None.""" with self._status_lock: - if self._output_model_instance is None: - return None - if isinstance(self._output_model_instance, LabThingsRootModelWrapper): - return self._output_model_instance.model_dump() - else: - return self._output_model_instance + return RootModelWrapper.unwrap(self._output_model_instance) @property def output_model_instance(self) -> BaseModel | None: @@ -289,7 +284,7 @@ def run(self) -> None: See `.Invocation.status` for status values. :raises RuntimeError: if there is no Thing associated with the invocation. - :raises InvalidReturnValue: if the action returns a value that can't + :raises InvalidReturnValueError: if the action returns a value that can't be validated by its output model. """ # self.action evaluates to an ActionDescriptor. This confuses mypy, @@ -323,6 +318,10 @@ def run(self) -> None: try: output_model_instance = action.output_model.model_validate(ret) + output_model_instance._labthings_created_as = ( + f"the output from {thing.name}.{action.name} " + f"invocation {self.id}." + ) except ValidationError as e: # Generate a helpful error message. This will be handled below, # where it will cause the action to be marked as failed, and the @@ -330,7 +329,7 @@ def run(self) -> None: msg = f"The return value from '{self.thing.name}.{action.name}' " msg += "failed to validate against its output model " msg += f"'{action.output_model}'. The return value was '{ret}'." - raise InvalidReturnValue(msg) from e + raise InvalidReturnValueError(msg) from e with self._status_lock: self._output_model_instance = output_model_instance @@ -343,7 +342,7 @@ def run(self) -> None: action.emit_changed_event(self.thing, self._status.value) except Exception as e: # skipcq: PYL-W0703 # First log - if isinstance(e, (InvocationError, InvalidReturnValue)): + if isinstance(e, (InvocationError, InvalidReturnValueError)): # Log without traceback for anticipated errors logger.error(e) elif ( @@ -738,6 +737,8 @@ def __init__( if more nuanced locking behaviour is required meaning the lock is acquired directly in the action code, for example using `~lt.ThingServerInterface.hold_global_lock`\ . + :raises UnserializableTypeError: if the return type of the action cannot + be serialised to JSON by `pydantic`\ . """ super().__init__() self.func = func @@ -754,7 +755,13 @@ def __init__( remove_first_positional_arg=True, ignore=[p.name for p in self.dependency_params], ) - self.output_model = wrap_plain_types_in_rootmodel(return_type(func)) + try: + self.output_model = RootModelWrapper.wrap_type( + return_type(func), name=f"{name.title()}Output" + ) + except UnserializableTypeError as e: + e.set_source_function(func) + raise self.invocation_model = create_model( f"{name}_invocation", __base__=InvocationModel, diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index bab67228..6dffe269 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -4,6 +4,8 @@ # An __all__ for this module is less than helpful, unless we have an # automated check that everything's included. +from collections.abc import Callable + class NotConnectedToServerError(RuntimeError): """The Thing is not connected to a server. @@ -254,7 +256,46 @@ class NoInvocationContextError(RuntimeError): """ -class InvalidReturnValue(RuntimeError): +class CausedByUserCodeError(Exception): + """A mixin to allow exceptions to refer to downstream code.""" + + def _append_to_args(self, message: str) -> None: + """Add a message to the exception's arguments. + + :param message: the message to append. + """ + if len(self.args) == 1: + # If there's only one string, assume it's a message and append + self.args = (self.args[0] + "\n" + message,) + else: + # If there are multiple arguments, add this as a further one + self.args += (message,) + + def set_source_function(self, func: Callable) -> None: + """Add the location of a user-supplied function to the error message. + + :param func: the function that caused this error. + """ + code = func.__code__ + self._append_to_args( + f"This was likely caused by function '{code.co_name}' " + f"at {code.co_filename}:{code.co_firstlineno}" + ) + + def set_source_class(self, cls: type, attr: str | None = None) -> None: + """Add a reference to a class (and optionally attribute). + + :param cls: the class that caused this error. + :param attr: the attribute name that caused this error. + """ + self._append_to_args( + f"This was likely caused by '{cls.__module__}.{cls.__qualname__}.{attr}" + if attr + else "'." + ) + + +class InvalidReturnValueError(CausedByUserCodeError, RuntimeError): r"""The return value from a method cannot be serialised by LabThings. This error is raised when an action returns a value that can't be serialised. @@ -271,6 +312,18 @@ class InvalidReturnValue(RuntimeError): """ +class UnserializableTypeError(CausedByUserCodeError, TypeError): + r"""A type has been specified that can't be serialized to JSON. + + This error generally means a property or action has a type that cannot be + serialized to JSON. This might be an instance of a custom class, or another + datatype that doesn't have a ready representation using JSON-compatible types. + + This error can often be fixed using `pydantic` annotations, or by using simple + Python types instead of custom ones. + """ + + class LogConfigurationError(RuntimeError): """There is a problem with logging configuration. diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index e3de1b6b..55297eb1 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -81,9 +81,8 @@ class attribute. Documentation is in strings immediately following the PropertyOp, ) from .utilities import ( - LabThingsRootModelWrapper, + RootModelWrapper, labthings_data, - wrap_plain_types_in_rootmodel, ) from .utilities.introspection import return_type from .base_descriptor import ( @@ -97,6 +96,7 @@ class attribute. Documentation is in strings immediately following the PropertyRedefinitionError, ReadOnlyPropertyError, MissingTypeError, + UnserializableTypeError, UnsupportedConstraintError, ) from .thing_class_settings import get_validate_properties_on_set @@ -464,12 +464,19 @@ def model(self) -> type[BaseModel]: subclass, this returns it unchanged. :return: a Pydantic model for the property's type. + :raises UnserializableTypeError: if the property can't be serialized + by `pydantic` to JSON. """ if self._model is None: - self._model = wrap_plain_types_in_rootmodel( - self.value_type, - constraints=self.constraints, - ) + try: + self._model = RootModelWrapper.wrap_type( + self.value_type, + constraints=self.constraints, + name=f"{self.name.title()}Value", + ) + except UnserializableTypeError as e: + e.set_source_class(self.owning_class, self.name) + raise return self._model def get_default(self, obj: Owner | None) -> Value: @@ -1270,7 +1277,7 @@ def validate(self, value: Any) -> Value: with its value type. This should never happen. """ try: - if issubclass(self.model, LabThingsRootModelWrapper): + if issubclass(self.model, RootModelWrapper): # If a plain type has been wrapped in a RootModel, use that to validate # and then set the property to the root value. model = self.model.model_validate(value) @@ -1283,7 +1290,7 @@ def validate(self, value: Any) -> Value: return self.value_type.model_validate(value) # This should be unreachable, because `model` is a - # `LabThingsRootModelWrapper` wrapping the value type, or the value type + # `RootModelWrapper` wrapping the value type, or the value type # should be a BaseModel. msg = f"Property {self.name} has an inconsistent model. This is " msg += f"most likely a LabThings bug. {self.model=}, {self.value_type=}" diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index 51515601..5fce3268 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -4,10 +4,24 @@ from collections.abc import Mapping from typing import Any, Dict, Generic, Iterable, TYPE_CHECKING, Optional, TypeVar from weakref import WeakSet -from pydantic import BaseModel, ConfigDict, Field, RootModel, create_model +from pydantic import ( + BaseModel, + ConfigDict, + Field, + RootModel, + create_model, + model_serializer, + SerializerFunctionWrapHandler, + PrivateAttr, + PydanticSchemaGenerationError, +) from pydantic.dataclasses import dataclass +from pydantic_core import PydanticSerializationError -from labthings_fastapi.exceptions import UnsupportedConstraintError +from labthings_fastapi.exceptions import ( + UnsupportedConstraintError, + UnserializableTypeError, +) from .introspection import EmptyObject if TYPE_CHECKING: @@ -17,9 +31,9 @@ __all__ = [ "class_attributes", "attributes", + "RootModelWrapper", "LabThingsObjectData", "labthings_data", - "wrap_plain_types_in_rootmodel", "model_to_dict", ] @@ -97,7 +111,7 @@ def labthings_data(obj: Thing) -> LabThingsObjectData: WrappedT = TypeVar("WrappedT") -class LabThingsRootModelWrapper(RootModel[WrappedT], Generic[WrappedT]): +class RootModelWrapper(RootModel[WrappedT], Generic[WrappedT]): """A RootModel subclass for automatically-wrapped types. There are several places where LabThings needs a model, but may only @@ -105,52 +119,106 @@ class LabThingsRootModelWrapper(RootModel[WrappedT], Generic[WrappedT]): a type has been automatically wrapped, and will need to be unwrapped in order for the value to have the correct type. - It has no additional functionality. + It also provides methods to automatically wrap types if they are not + already `pydantic.BaseModel` subclasses, and to unwrap them again, and + there is provision to add metadata that makes it easier to locate errors + if serialisation fails. """ - -def wrap_plain_types_in_rootmodel( - model: type, constraints: Mapping[str, Any] | None = None -) -> type[BaseModel]: - """Ensure a type is a subclass of BaseModel. - - If a `pydantic.BaseModel` subclass is passed to this function, we will pass it - through unchanged. Otherwise, we wrap the type in a `pydantic.RootModel`. - In the future, we may explicitly check that the argument is a type - and not a model instance. - - :param model: A Python type or `pydantic` model. - :param constraints: is passed as keyword arguments to `pydantic.Field` - to add validation constraints to the property. - - :return: A `pydantic` model, wrapping Python types in a ``RootModel`` if needed. - - :raises UnsupportedConstraintError: if constraints are provided for an - unsuitable type, for example `allow_inf_nan` for an `int` property, or - any constraints for a `BaseModel` subclass. - :raises RuntimeError: if other errors prevent Pydantic from creating a schema - for the generated model. - """ - try: # This needs to be a `try` as basic types are not classes - if issubclass(model, BaseModel): - if constraints: - raise UnsupportedConstraintError( - "Constraints may only be applied to plain types, not Models." - ) - return model - except TypeError: - pass # some plain types aren't classes and that's OK - they still get wrapped. - constraints = constraints or {} - try: - return create_model( - f"{model!r}", - root=(model, Field(**constraints)), - __base__=LabThingsRootModelWrapper, - ) - except RuntimeError as e: - if "Unable to apply constraint" in str(e): - raise UnsupportedConstraintError(str(e)) from e - raise e + _labthings_created_as: str | None = PrivateAttr(default=None) + + @classmethod + def wrap_type( + cls, + model: type, + constraints: Mapping[str, Any] | None = None, + name: str | None = None, + ) -> type[BaseModel]: + r"""Ensure a type is a subclass of BaseModel. + + If a `pydantic.BaseModel` subclass is passed to this function, we will pass it + through unchanged. Otherwise, we wrap the type in a `pydantic.RootModel`. + In the future, we may explicitly check that the argument is a type + and not a model instance. + + :param model: A Python type or `pydantic` model. + :param constraints: is passed as keyword arguments to `pydantic.Field` + to add validation constraints to the property. + :param name: the name to use for the dynamically created model. + + :return: A `pydantic` model, wrapping Python types in a ``RootModel`` if needed. + + :raises UnsupportedConstraintError: if constraints are provided for an + unsuitable type, for example `allow_inf_nan` for an `int` property, or + any constraints for a `BaseModel` subclass. + :raises UnserializableTypeError: if the type being wrapped is not able + to be serialized by `pydantic`\ . + :raises RuntimeError: if other errors prevent Pydantic from creating a schema + for the generated model. + """ + try: # This needs to be a `try` as basic types are not classes + if issubclass(model, BaseModel): + if constraints: + raise UnsupportedConstraintError( + "Constraints may only be applied to plain types, not Models." + ) + return model + except TypeError: + pass # some types aren't classes and that's OK - they still get wrapped. + constraints = constraints or {} + try: + # Dynamically create a subclass of RootModelWrapper for the supplied type. + return create_model( + f"{name or cls.__name__}[{model!r}]", + root=(model, Field(**constraints)), + __base__=cls, + ) + except PydanticSchemaGenerationError as e: + raise UnserializableTypeError( + f"LabThings does not know how to serialize {model!r} to JSON." + ) from e + except RuntimeError as e: + if "Unable to apply constraint" in str(e): + raise UnsupportedConstraintError(str(e)) from e + raise e + + @classmethod + def unwrap(cls, value: BaseModel | None) -> Any: + r"""If the supplied value is a `RootModelWrapper`, unwrap it. + + :param value: a model instance. + :return: the root value, if ``value`` is a `RootModelWrapper`\ , or ``value`` + if not. + """ + if value is None: + return None + if isinstance(value, cls): + return value.root + return value + + @model_serializer(mode="wrap") + def add_context_to_serialization( + self, handler: SerializerFunctionWrapHandler + ) -> Any: + """Ensure that serialization errors are accompanied by context. + + This wraps Pydantic's serialization error in a custom error that makes it clear + where the problematic model originated. + + :param handler: the underlying Pydantic serializer. + :return: a JSONable value. + :raises ValueError: if the serialization fails. This wraps the underlying + `pydantic` error to provide additional context. + """ + try: + return handler(self) + except PydanticSerializationError as e: + purpose = self._labthings_created_as + raise ValueError( + f"There was an error serializing {self!r}, created as {purpose} " + if purpose + else ". The serialization error was '{e}'." + ) from e def model_to_dict(model: Optional[BaseModel]) -> Dict[str, Any]: From f274a5dc9025c1ef853a14c6aebea9ee453ea05e Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 11 May 2026 23:23:21 +0100 Subject: [PATCH 06/18] Take control of property serialisation This commit moves both validation and serialisation of properties into LabThings code, so the exception may be handled properly. If this looks good, we may want to make a custom Response class. --- src/labthings_fastapi/actions.py | 4 -- src/labthings_fastapi/properties.py | 43 +++++++++++++++++++-- src/labthings_fastapi/utilities/__init__.py | 8 ++-- 3 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 41080db4..1ff7cafb 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -318,10 +318,6 @@ def run(self) -> None: try: output_model_instance = action.output_model.model_validate(ret) - output_model_instance._labthings_created_as = ( - f"the output from {thing.name}.{action.name} " - f"invocation {self.id}." - ) except ValidationError as e: # Generate a helpful error message. This will be handled below, # where it will cause the action to be marked as failed, and the diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 55297eb1..fdc85ade 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -48,6 +48,7 @@ class attribute. Documentation is in strings immediately following the from __future__ import annotations import builtins from collections.abc import Mapping +import inspect from types import EllipsisType from typing import ( Annotated, @@ -62,7 +63,8 @@ class attribute. Documentation is in strings immediately following the from typing_extensions import Self, TypedDict from weakref import WeakSet -from fastapi import Body, FastAPI +from fastapi import Body, FastAPI, Response +from fastapi.responses import JSONResponse from pydantic import ( BaseModel, ConfigDict, @@ -72,6 +74,7 @@ class attribute. Documentation is in strings immediately following the create_model, with_config, ) +from pydantic_core import PydanticSerializationError from .thing_description import type_to_dataschema from .thing_description._model import ( @@ -566,8 +569,42 @@ def set_property(body: Any) -> None: summary=self.title, description=f"## {self.title}\n\n{self.description or ''}", ) - def get_property() -> Any: - return self.__get__(thing) + def get_property() -> Response: + try: + value = self.__get__(thing) + model = self.model.model_validate(value) + except ValidationError as e: + filename = inspect.getsourcefile(self.owning_class) + msg = f"Could not validate the value of {thing.name}.{self.name} " + msg += "against its model.\n" + msg += f"The value was '{value}' and the model was {self.model!r}\n" + msg += f"The validation error was {e}.\n" + msg += f"See '{self.owning_class.__qualname__}.{self.name}' " + msg += f"defined in {filename}." + thing.logger.error(msg) + return JSONResponse( + status_code=500, + content={"detail": msg}, + ) + try: + return Response( + content=model.model_dump_json(), + status_code=200, + media_type="application/json", + ) + except PydanticSerializationError as e: + filename = inspect.getsourcefile(self.owning_class) + msg = f"Could not serialise the value of {thing.name}.{self.name} " + msg += "to JSON.\n" + msg += f"It was validated as {model!r}.\n" + msg += f"The serialization error was {e}.\n" + msg += f"See '{self.owning_class.__qualname__}.{self.name}' " + msg += f"defined in {filename}." + thing.logger.error(msg) + return JSONResponse( + status_code=500, + content={"detail": msg}, + ) if self.is_resettable(thing): diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index 5fce3268..ac925ded 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -207,17 +207,17 @@ def add_context_to_serialization( :param handler: the underlying Pydantic serializer. :return: a JSONable value. - :raises ValueError: if the serialization fails. This wraps the underlying - `pydantic` error to provide additional context. + :raises PydanticSerializationError: if the serialization fails. This wraps the + underlying `pydantic` error to provide additional context. """ try: return handler(self) except PydanticSerializationError as e: purpose = self._labthings_created_as - raise ValueError( + raise PydanticSerializationError( f"There was an error serializing {self!r}, created as {purpose} " if purpose - else ". The serialization error was '{e}'." + else f". The serialization error was '{e}'." ) from e From 69cea301f6571b01e92a9396d50d5f027636118d Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 11:24:58 +0100 Subject: [PATCH 07/18] Disable FastAPI schema splitting This was causing properties to have no schema for their GET endpoint. It's detailed at https://fastapi.tiangolo.com/how-to/separate-openapi-schemas/#same-schema-for-input-and-output-models-in-docs --- src/labthings_fastapi/server/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 3771d7bd..18871594 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -145,7 +145,7 @@ def __init__( self._config = ThingServerConfig(**kwargs) if self._config.settings_folder is None: self._config.settings_folder = "./settings" - self.app = FastAPI(lifespan=self.lifespan) + self.app = FastAPI(lifespan=self.lifespan, separate_input_output_schemas=False) self._set_cors_middleware() self._set_url_for_middleware() self._add_exception_handlers() From b4a07a627d246861c42d0716d507f046be8d8cd9 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 11:27:28 +0100 Subject: [PATCH 08/18] Move error handling into `response_from_user_code`. This commit removes the complicated wrapper-based error handling in favour of a function. This approach is specific (only catches ValidationError/SerializationError directly related to user code) and avoids our custom errors being re-wrapped by `pydantic`. --- src/labthings_fastapi/properties.py | 46 +----- src/labthings_fastapi/utilities/__init__.py | 163 ++++++++++++++++---- 2 files changed, 142 insertions(+), 67 deletions(-) diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index fdc85ade..665de313 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -48,7 +48,6 @@ class attribute. Documentation is in strings immediately following the from __future__ import annotations import builtins from collections.abc import Mapping -import inspect from types import EllipsisType from typing import ( Annotated, @@ -64,7 +63,6 @@ class attribute. Documentation is in strings immediately following the from weakref import WeakSet from fastapi import Body, FastAPI, Response -from fastapi.responses import JSONResponse from pydantic import ( BaseModel, ConfigDict, @@ -74,7 +72,6 @@ class attribute. Documentation is in strings immediately following the create_model, with_config, ) -from pydantic_core import PydanticSerializationError from .thing_description import type_to_dataschema from .thing_description._model import ( @@ -86,6 +83,7 @@ class attribute. Documentation is in strings immediately following the from .utilities import ( RootModelWrapper, labthings_data, + response_from_user_code, ) from .utilities.introspection import return_type from .base_descriptor import ( @@ -570,41 +568,13 @@ def set_property(body: Any) -> None: description=f"## {self.title}\n\n{self.description or ''}", ) def get_property() -> Response: - try: - value = self.__get__(thing) - model = self.model.model_validate(value) - except ValidationError as e: - filename = inspect.getsourcefile(self.owning_class) - msg = f"Could not validate the value of {thing.name}.{self.name} " - msg += "against its model.\n" - msg += f"The value was '{value}' and the model was {self.model!r}\n" - msg += f"The validation error was {e}.\n" - msg += f"See '{self.owning_class.__qualname__}.{self.name}' " - msg += f"defined in {filename}." - thing.logger.error(msg) - return JSONResponse( - status_code=500, - content={"detail": msg}, - ) - try: - return Response( - content=model.model_dump_json(), - status_code=200, - media_type="application/json", - ) - except PydanticSerializationError as e: - filename = inspect.getsourcefile(self.owning_class) - msg = f"Could not serialise the value of {thing.name}.{self.name} " - msg += "to JSON.\n" - msg += f"It was validated as {model!r}.\n" - msg += f"The serialization error was {e}.\n" - msg += f"See '{self.owning_class.__qualname__}.{self.name}' " - msg += f"defined in {filename}." - thing.logger.error(msg) - return JSONResponse( - status_code=500, - content={"detail": msg}, - ) + return response_from_user_code( + model=self.model, + value=self.__get__(thing), + description=f"{thing.name}.{self.name}", + logger=thing.logger, + code=(self.owning_class, self.name), + ) if self.is_resettable(thing): diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index ac925ded..dfd953e8 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -2,6 +2,8 @@ from __future__ import annotations from collections.abc import Mapping +import logging +from types import FunctionType from typing import Any, Dict, Generic, Iterable, TYPE_CHECKING, Optional, TypeVar from weakref import WeakSet from pydantic import ( @@ -10,15 +12,16 @@ Field, RootModel, create_model, - model_serializer, - SerializerFunctionWrapHandler, - PrivateAttr, PydanticSchemaGenerationError, + ValidationError, ) from pydantic.dataclasses import dataclass from pydantic_core import PydanticSerializationError +from fastapi import Response +from fastapi.responses import JSONResponse from labthings_fastapi.exceptions import ( + InvalidReturnValueError, UnsupportedConstraintError, UnserializableTypeError, ) @@ -120,13 +123,9 @@ class RootModelWrapper(RootModel[WrappedT], Generic[WrappedT]): in order for the value to have the correct type. It also provides methods to automatically wrap types if they are not - already `pydantic.BaseModel` subclasses, and to unwrap them again, and - there is provision to add metadata that makes it easier to locate errors - if serialisation fails. + already `pydantic.BaseModel` subclasses, and to unwrap them again. """ - _labthings_created_as: str | None = PrivateAttr(default=None) - @classmethod def wrap_type( cls, @@ -196,29 +195,135 @@ def unwrap(cls, value: BaseModel | None) -> Any: return value.root return value - @model_serializer(mode="wrap") - def add_context_to_serialization( - self, handler: SerializerFunctionWrapHandler - ) -> Any: - """Ensure that serialization errors are accompanied by context. - This wraps Pydantic's serialization error in a custom error that makes it clear - where the problematic model originated. +def refer_to_user_code( + code: FunctionType | tuple[type, str] | None = None, suffix: str = "\n" +) -> str: + r"""Refer to a user-supplied function or property. - :param handler: the underlying Pydantic serializer. - :return: a JSONable value. - :raises PydanticSerializationError: if the serialization fails. This wraps the - underlying `pydantic` error to provide additional context. - """ - try: - return handler(self) - except PydanticSerializationError as e: - purpose = self._labthings_created_as - raise PydanticSerializationError( - f"There was an error serializing {self!r}, created as {purpose} " - if purpose - else f". The serialization error was '{e}'." - ) from e + This function generates a human-readable error string that should enable someone + to find a problem in downstream code. + + If `code` is `None` the empty string will be returned. This is intended to simplify + the construction of error messages that may or may not include a code location. + + :param code: the code that generated `value`\ . This may be either a function, + a tuple consisting of a class and an attribute name, or a string (which + should describe how to find the user code that generated the value). + :param suffix: a string that terminates the message, by default a newline. This + is not used if `code` is None, and instead the empty string is returned. + :return: a string referring to the user code, for use in an error message, or + the empty string if no user code is specified. + """ + if isinstance(code, FunctionType): + co = code.__code__ + return ( + f"This value was returned by '{co.co_name}' " + f"at {co.co_filename}:{co.co_firstlineno}.{suffix}" + ) + elif isinstance(code, tuple) and len(code) == 2: + cls, attr = code + return ( + "You may want to check the definition of " + f"{cls.__module__}.{cls.__qualname__}.{attr}.{suffix}" + ) + else: + return "" + + +ModelT = TypeVar("ModelT", bound=BaseModel) + + +def validate_from_user_code( + model: type[ModelT], + value: Any, + description: str, + code: FunctionType | tuple[type, str] | None = None, +) -> ModelT: + r"""Validate a return value from user code, with error handling. + + This wraps ``return model.model_validate(value)`` in error handling code. + It is intended to help LabThings generate better errors when models fail + to validate, in particular making clear where in the user's code the value + was generated, and why it didn't validate. + + :param model: the `pydantic` model to use for validation. + :param value: the value passed to ``model.model_validate()``\ . + :param description: a description of the value, e.g. "the output of {action}". + :param code: the code that generated `value`\ . + This will be passed to `refer_to_user_code` - see that function for details. + + :return: a validated model instance. + :raises InvalidReturnValueError: if the model failed to validate. + """ + try: + return model.model_validate(value) + except ValidationError as e: + msg = ( + f"Error validating {description} against its model.\n" + f"The value was '{value}' and the model was {model}.\n" + f"{refer_to_user_code(code)}" + f"The validation error was:\n{e}\n" + ) + raise InvalidReturnValueError(msg) from e + + +def response_from_user_code( + model: type[ModelT], + value: Any, + description: str, + logger: logging.Logger, + code: FunctionType | tuple[type, str] | None = None, +) -> Response: + r"""Return a value from a `~lt.Thing` method, with appropriate error handling. + + This function implements very similar logic to FastAPI's default behaviour when + an endpoint function is typed as returning a `pydantic.BaseModel` instance: + + * First, a model instance is constructed by calling ``model_validate()`` on the + return value. + * Second, the validated model instance is serialised to JSON by calling + ``model_dump_json()`` on the model instance. + + If either of these steps fails, an unhelpful stack trace is dumped, which has many + frames referring to FastAPI/Pydantic/Starlette internals and nothing to indicate + where the problematic value actually came from. + + This function wraps both the steps described above in error handling code that + should ensure that, if either fails, we write a helpful error into the log and + return a `500` response with the same message in its body. + + :param model: the `pydantic` model to use for validation. + :param value: the value passed to ``model.model_validate()``\ . + :param description: a description of the value, e.g. "the output of {action}". + :param logger: the logger to use for any error messages. + :param code: the code that generated `value`\ . + This will be passed to `refer_to_user_code` - see that function for details. + :return: a `fastapi.Response` object containing a 200 code and the serialised + value or a 500 code and the error message. + """ + try: + model_instance = validate_from_user_code(model, value, description, code) + except InvalidReturnValueError as exc: + # Log the error, but we don't want a stack trace - it won't tell + # the user anything about where in their code the problem is. + logger.error(str(exc)) + return JSONResponse(status_code=500, content={"detail": str(exc)}) + try: + return Response( + content=model_instance.model_dump_json(), + status_code=200, + media_type="application/json", + ) + except PydanticSerializationError as exc: + msg = ( + f"Error serializing {description} to JSON.\n" + f"The value was validated as {repr(model_instance)}.\n" + f"The serialization error was '{exc}'.\n" + f"{refer_to_user_code(code)}" + ) + logger.error(msg) + return JSONResponse(status_code=500, content={"detail": msg}) def model_to_dict(model: Optional[BaseModel]) -> Dict[str, Any]: From 6cce030bae21fc1460f8f63ce3428fe380f5f270 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 14:00:46 +0100 Subject: [PATCH 09/18] Manually serialise values from Thing code. This makes a few changes to ensure we handle serialization errors better: 1. Invocations now only return their input, output, log, and links when accessed individually. This avoids the possibility of serialization errors in lists of invocations (and should give a decent performance boost). 2. Endpoints that return values from downstream code (property reads or action outputs) now return `Response` directly, and use `serialize_from_user_code` to apply sensible error handling. 3. Everywhere I've used `serialize_from_user_code` I've specified the model in the FastAPI decorator, and ensured the exception is handled, logged, and re-raised as an HTTPException. I've split up `response_from_user_code` to handle validation and serialization separately: that fits better with the way actions are structured. --- src/labthings_fastapi/actions.py | 125 ++++++++++++-------- src/labthings_fastapi/invocations.py | 54 ++++----- src/labthings_fastapi/properties.py | 31 +++-- src/labthings_fastapi/utilities/__init__.py | 74 +++++------- tests/test_action_manager.py | 3 + tests/test_actions.py | 13 +- 6 files changed, 159 insertions(+), 141 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 1ff7cafb..2e7d0df9 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -39,8 +39,8 @@ ) from weakref import WeakSet import weakref -from fastapi import APIRouter, FastAPI, HTTPException, Request, Body, BackgroundTasks -from pydantic import BaseModel, create_model, ValidationError +from fastapi import APIRouter, FastAPI, HTTPException, Response, Body, BackgroundTasks +from pydantic import BaseModel, create_model from .middleware.url_for import URLFor from .base_descriptor import ( @@ -52,8 +52,10 @@ from .utilities import ( RootModelWrapper, model_to_dict, + serialize_from_user_code, + validate_from_user_code, ) -from .invocations import InvocationModel, InvocationStatus +from .invocations import InvocationSummary, InvocationModel, InvocationStatus from .exceptions import ( GlobalLockBusyError, InvalidReturnValueError, @@ -226,6 +228,22 @@ def cancel(self) -> None: """ self.cancel_hook.set() + def summary_model(self) -> InvocationSummary: + """Generate a summary of the invocation suitable for HTTP. + + :return: a `InvocationSummary` representing this `Invocation`. + """ + with self._status_lock: + return InvocationSummary( + status=self.status, + id=self.id, + action=self.thing.path + self.action.name, # type: ignore[attr-defined] + href=URLFor("action_invocation", id=self.id), + timeStarted=self._start_time, + timeCompleted=self._end_time, + timeRequested=self._request_time, + ) + def response(self) -> InvocationModel: """Generate a representation of the invocation suitable for HTTP. @@ -245,13 +263,7 @@ def response(self) -> InvocationModel: # object (i.e. we don't call __get__ on the descriptor). with self._status_lock: return self.action.invocation_model( # type: ignore[attr-defined] - status=self.status, - id=self.id, - action=self.thing.path + self.action.name, # type: ignore[attr-defined] - href=URLFor("action_invocation", id=self.id), - timeStarted=self._start_time, - timeCompleted=self._end_time, - timeRequested=self._request_time, + **dict(self.summary_model()), input=self.input, output=self.output_model_instance, links=links, @@ -284,8 +296,6 @@ def run(self) -> None: See `.Invocation.status` for status values. :raises RuntimeError: if there is no Thing associated with the invocation. - :raises InvalidReturnValueError: if the action returns a value that can't - be validated by its output model. """ # self.action evaluates to an ActionDescriptor. This confuses mypy, # which thinks we are calling ActionDescriptor.__get__. @@ -316,16 +326,12 @@ def run(self) -> None: # Actually run the action ret = action.func(thing, **kwargs, **self.dependencies) - try: - output_model_instance = action.output_model.model_validate(ret) - except ValidationError as e: - # Generate a helpful error message. This will be handled below, - # where it will cause the action to be marked as failed, and the - # error will end up in the log. - msg = f"The return value from '{self.thing.name}.{action.name}' " - msg += "failed to validate against its output model " - msg += f"'{action.output_model}'. The return value was '{ret}'." - raise InvalidReturnValueError(msg) from e + output_model_instance = validate_from_user_code( + model=action.output_model, + value=ret, + description=f"the output of '{self.thing.name}.{action.name}'", + code=action.func, + ) with self._status_lock: self._output_model_instance = output_model_instance @@ -435,11 +441,10 @@ def list_invocations( self, action: Optional[ActionDescriptor] = None, thing: Optional[Thing] = None, - request: Optional[Request] = None, - ) -> list[InvocationModel]: + ) -> list[InvocationSummary]: """All of the invocations currently managed. - Returns a list of `.InvocationModel` instances representing all the + Returns a list of `InvocationSummary` instances representing all the invocations that are currently running, or have recently completed and not yet expired. @@ -450,16 +455,11 @@ def list_invocations( :param thing: returns only invocations of actions on a particular `~lt.Thing`. This will often be combined with filtering by ``action`` to give the list of invocations returned by a GET request on an action endpoint. - :param request: is used to pass a `fastapi.Request` object to the - `.Invocation.response` method. Doing so ensures the URL returned as - ``href`` in the response matches the address used to communicate with - the server (i.e. it uses `fastapi.Request.url_for` instead of a path - generated from a string). :return: A list of invocations, optionally filtered by Thing and/or Action. """ return [ - i.response() + i.summary_model() for i in self.invocations if thing is None or i.thing == thing if action is None or i.action == action @@ -484,20 +484,19 @@ def router(self) -> APIRouter: """ router = APIRouter() - @router.get(ACTION_INVOCATIONS_PATH, response_model=list[InvocationModel]) - def list_all_invocations(request: Request) -> list[InvocationModel]: - return self.list_invocations(request=request) + @router.get(ACTION_INVOCATIONS_PATH) + def list_all_invocations() -> list[InvocationSummary]: + return self.list_invocations() @router.get( ACTION_INVOCATIONS_PATH + "/{id}", + response_model=InvocationModel, responses={404: {"description": "Invocation ID not found"}}, ) - def action_invocation(id: uuid.UUID, request: Request) -> InvocationModel: + def action_invocation(id: uuid.UUID) -> Response: """Return a description of a specific action. :param id: The action's ID (from the path). - :param request: FastAPI dependency for the request object, used to - find URLs via ``url_for``. :return: Details of the invocation. @@ -505,13 +504,23 @@ def action_invocation(id: uuid.UUID, request: Request) -> InvocationModel: found. """ try: - with self._invocations_lock: - return self._invocations[id].response() + invocation = self.get_invocation(id) + return serialize_from_user_code( + model_instance=invocation.response(), + description=f"invocation '{id}' of ", + code=invocation.action.func, # type: ignore[attr-defined] + ) except KeyError as e: raise HTTPException( status_code=404, detail="No action invocation found with ID {id}", ) from e + except InvalidReturnValueError as e: + invocation.thing.logger.error(e) + raise HTTPException( + status_code=500, + detail=str(e), + ) from e @router.get( ACTION_INVOCATIONS_PATH + "/{id}/output", @@ -527,7 +536,7 @@ def action_invocation(id: uuid.UUID, request: Request) -> InvocationModel: 503: {"description": "No result is available for this invocation"}, }, ) - def action_invocation_output(id: uuid.UUID) -> Any: + def action_invocation_output(id: uuid.UUID) -> Response: """Get the output of an action invocation. This returns just the "output" component of the action invocation. If the @@ -559,7 +568,18 @@ def action_invocation_output(id: uuid.UUID) -> Any: ): # TODO: honour "accept" header return invocation.output.response() - return invocation.output_model_instance + try: + return serialize_from_user_code( + model_instance=invocation.output_model_instance, + description=f"the output of {invocation}", + code=invocation.action.func, + ) + except InvalidReturnValueError as e: + invocation.thing.logger.error(e) + raise HTTPException( + status_code=500, + detail=str(e), + ) from e @router.delete( ACTION_INVOCATIONS_PATH + "/{id}", @@ -841,7 +861,7 @@ def instance_get(self, obj: OwnerT) -> Callable[ActionParams, ActionReturn]: """ @wraps(self.func) - def wrapped(*args: Any, **kwargs: Any) -> Any: # noqa: DOC + def wrapped(*args: Any, **kwargs: Any) -> Any: # noqa: DOC101, DOC103, DOC201 """Acquire the lock then run `func` with supplied arguments.""" with self.context_for_func(obj): return self.func(*args, **kwargs) @@ -931,16 +951,25 @@ def start_action( body: Any, # This annotation will be overwritten below. background_tasks: BackgroundTasks, **dependencies: Any, - ) -> InvocationModel: + ) -> Response: action_manager = thing._thing_server_interface._action_manager - action = action_manager.invoke_action( + invocation = action_manager.invoke_action( action=self, thing=thing, input=body, dependencies=dependencies, ) background_tasks.add_task(action_manager.expire_invocations) - return action.response() + try: + return serialize_from_user_code( + model_instance=invocation.response(), + description=f"{invocation}", + status_code=201, + code=self.func, + ) + except InvalidReturnValueError as e: + thing.logger.error(e) + raise HTTPException(status_code=500, detail=str(e)) from e if issubclass(self.input_model, EmptyInput): annotation = Body(default_factory=StrictEmptyInput) @@ -983,9 +1012,9 @@ def start_action( ) app.post( thing.path + self.name, - response_model=self.invocation_model, status_code=201, response_description="Action has been invoked (and may still be running).", + response_model=self.invocation_model, description=f"## {self.title}\n\n {self.description} {ACTION_POST_NOTICE}", summary=self.title, responses=responses, @@ -993,7 +1022,7 @@ def start_action( @app.get( thing.path + self.name, - response_model=list[self.invocation_model], # type: ignore + response_model=list[InvocationSummary], # type: ignore # MyPy doesn't like the line above - but it works for FastAPI # to generate a response model. response_description=f"A list of every invocation of {self.name}.", @@ -1002,7 +1031,7 @@ def start_action( ), summary=f"All invocations of {self.name}.", ) - def list_invocations() -> list[InvocationModel]: + def list_invocations() -> list[InvocationSummary]: action_manager = thing._thing_server_interface._action_manager return action_manager.list_invocations(self, thing) diff --git a/src/labthings_fastapi/invocations.py b/src/labthings_fastapi/invocations.py index e010c5d5..e61e6125 100644 --- a/src/labthings_fastapi/invocations.py +++ b/src/labthings_fastapi/invocations.py @@ -15,10 +15,7 @@ BaseModel, ConfigDict, model_validator, - model_serializer, - SerializerFunctionWrapHandler, ) -from pydantic_core import PydanticSerializationError from labthings_fastapi.middleware.url_for import URLFor @@ -87,11 +84,30 @@ def generate_message(cls, data: Any) -> Any: return data +class InvocationSummary(BaseModel): + """A model to represent `.Invocation` objects over HTTP. + + This version of the model does not include logs our action outputs, and is intended + for use in endpoints that might list several invocations. + + See `GenericInvocationModel` for the full representation, to be used in + endpoints referring to one specific invocation. + """ + + status: InvocationStatus + id: uuid.UUID + action: str + href: URLFor + timeStarted: Optional[datetime] + timeRequested: Optional[datetime] + timeCompleted: Optional[datetime] + + InputT = TypeVar("InputT") OutputT = TypeVar("OutputT") -class GenericInvocationModel(BaseModel, Generic[InputT, OutputT]): +class GenericInvocationModel(InvocationSummary, Generic[InputT, OutputT]): """A model to serialise `.Invocation` objects when they are polled over HTTP. The input and output models are generic parameters, to allow this model to @@ -100,41 +116,11 @@ class GenericInvocationModel(BaseModel, Generic[InputT, OutputT]): are not known in advance. """ - status: InvocationStatus - id: uuid.UUID - action: str - href: URLFor - timeStarted: Optional[datetime] - timeRequested: Optional[datetime] - timeCompleted: Optional[datetime] input: InputT output: OutputT log: Sequence[LogRecordModel] links: Links = None - @model_serializer(mode="wrap") - def serialize_model( - self, handler: SerializerFunctionWrapHandler - ) -> dict[str, object]: - """Give a more helpful error if the class fails to serialize. - - :param handler: The Pydantic serializer. - :raises PydanticSerializationError: if the model fails to serialize. This - is wrapped to add the action and invocation ID. - :return: the serialized model, as a dictionary. - """ - try: - return handler(self) - except PydanticSerializationError as e: - extra = "" - if self.output is not None: - extra = "This is often caused by an invalid return value. " - raise PydanticSerializationError( - f"Could not serialise invocation '{self.id}' of '{self.action}' " - f"({self.status.value}). {extra}" - f"Error: '{e}'" - ) from e - InvocationModel = GenericInvocationModel[Any, Any] """A model to serialise `.Invocation` objects when they are polled over HTTP.""" diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 665de313..a51295ba 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -62,7 +62,7 @@ class attribute. Documentation is in strings immediately following the from typing_extensions import Self, TypedDict from weakref import WeakSet -from fastapi import Body, FastAPI, Response +from fastapi import Body, FastAPI, Response, HTTPException from pydantic import ( BaseModel, ConfigDict, @@ -83,7 +83,8 @@ class attribute. Documentation is in strings immediately following the from .utilities import ( RootModelWrapper, labthings_data, - response_from_user_code, + serialize_from_user_code, + validate_from_user_code, ) from .utilities.introspection import return_type from .base_descriptor import ( @@ -93,6 +94,7 @@ class attribute. Documentation is in strings immediately following the ) from .exceptions import ( FeatureNotAvailableError, + InvalidReturnValueError, NotConnectedToServerError, PropertyRedefinitionError, ReadOnlyPropertyError, @@ -568,13 +570,24 @@ def set_property(body: Any) -> None: description=f"## {self.title}\n\n{self.description or ''}", ) def get_property() -> Response: - return response_from_user_code( - model=self.model, - value=self.__get__(thing), - description=f"{thing.name}.{self.name}", - logger=thing.logger, - code=(self.owning_class, self.name), - ) + try: + instance = validate_from_user_code( + model=self.model, + value=self.__get__(thing), + description=f"{thing.name}.{self.name}", + code=(self.owning_class, self.name), + ) + return serialize_from_user_code( + model_instance=instance, + description=f"{thing.name}.{self.name}", + code=(self.owning_class, self.name), + ) + except InvalidReturnValueError as e: + thing.logger.error(e) + raise HTTPException( + status_code=500, + detail=str(e), + ) from e if self.is_resettable(thing): diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index dfd953e8..91a9937e 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -1,8 +1,7 @@ """Utility functions used by LabThings-FastAPI.""" from __future__ import annotations -from collections.abc import Mapping -import logging +from collections.abc import Callable, Mapping from types import FunctionType from typing import Any, Dict, Generic, Iterable, TYPE_CHECKING, Optional, TypeVar from weakref import WeakSet @@ -18,7 +17,6 @@ from pydantic.dataclasses import dataclass from pydantic_core import PydanticSerializationError from fastapi import Response -from fastapi.responses import JSONResponse from labthings_fastapi.exceptions import ( InvalidReturnValueError, @@ -197,7 +195,7 @@ def unwrap(cls, value: BaseModel | None) -> Any: def refer_to_user_code( - code: FunctionType | tuple[type, str] | None = None, suffix: str = "\n" + code: Callable | tuple[type, str] | None = None, suffix: str = "\n" ) -> str: r"""Refer to a user-supplied function or property. @@ -215,12 +213,17 @@ def refer_to_user_code( :return: a string referring to the user code, for use in an error message, or the empty string if no user code is specified. """ - if isinstance(code, FunctionType): - co = code.__code__ - return ( - f"This value was returned by '{co.co_name}' " - f"at {co.co_filename}:{co.co_firstlineno}.{suffix}" - ) + if callable(code): + if isinstance(code, FunctionType): + # As a rule, we'll pass a function and this code works. + co = code.__code__ + return ( + f"This value was returned by '{co.co_name}' " + f"at {co.co_filename}:{co.co_firstlineno}.{suffix}" + ) + else: + # As a fallback (not currently used), just dump the object to string. + return f"This value was returned by {repr(code)}.{suffix}" elif isinstance(code, tuple) and len(code) == 2: cls, attr = code return ( @@ -238,7 +241,7 @@ def validate_from_user_code( model: type[ModelT], value: Any, description: str, - code: FunctionType | tuple[type, str] | None = None, + code: Callable | tuple[type, str] | None = None, ) -> ModelT: r"""Validate a return value from user code, with error handling. @@ -268,51 +271,37 @@ def validate_from_user_code( raise InvalidReturnValueError(msg) from e -def response_from_user_code( - model: type[ModelT], - value: Any, +def serialize_from_user_code( + model_instance: BaseModel, description: str, - logger: logging.Logger, - code: FunctionType | tuple[type, str] | None = None, + status_code: int = 200, + code: Callable | tuple[type, str] | None = None, ) -> Response: - r"""Return a value from a `~lt.Thing` method, with appropriate error handling. + r"""Return a value from a model instance, with appropriate error handling. This function implements very similar logic to FastAPI's default behaviour when - an endpoint function is typed as returning a `pydantic.BaseModel` instance: + an endpoint function is typed as returning a `pydantic.BaseModel` instance. + The validated model instance is serialised to JSON by calling + ``model_dump_json()`` on the model instance, and the resulting string is returned + in a `Response` object. This uses `pydantic` serialization, written in Rust, + and outperforms the native `json` library significantly. - * First, a model instance is constructed by calling ``model_validate()`` on the - return value. - * Second, the validated model instance is serialised to JSON by calling - ``model_dump_json()`` on the model instance. + If the model can't be serialized, we raise an exception with information about + the place in the user code where the problem occurred. - If either of these steps fails, an unhelpful stack trace is dumped, which has many - frames referring to FastAPI/Pydantic/Starlette internals and nothing to indicate - where the problematic value actually came from. - - This function wraps both the steps described above in error handling code that - should ensure that, if either fails, we write a helpful error into the log and - return a `500` response with the same message in its body. - - :param model: the `pydantic` model to use for validation. - :param value: the value passed to ``model.model_validate()``\ . + :param model_instance: the `pydantic` model to use for validation. :param description: a description of the value, e.g. "the output of {action}". - :param logger: the logger to use for any error messages. + :param status_code: an HTTP status code to use. :param code: the code that generated `value`\ . This will be passed to `refer_to_user_code` - see that function for details. :return: a `fastapi.Response` object containing a 200 code and the serialised value or a 500 code and the error message. + :raises InvalidReturnValueError: if the model can't be serialised. """ - try: - model_instance = validate_from_user_code(model, value, description, code) - except InvalidReturnValueError as exc: - # Log the error, but we don't want a stack trace - it won't tell - # the user anything about where in their code the problem is. - logger.error(str(exc)) - return JSONResponse(status_code=500, content={"detail": str(exc)}) try: return Response( content=model_instance.model_dump_json(), - status_code=200, + status_code=status_code, media_type="application/json", ) except PydanticSerializationError as exc: @@ -322,8 +311,7 @@ def response_from_user_code( f"The serialization error was '{exc}'.\n" f"{refer_to_user_code(code)}" ) - logger.error(msg) - return JSONResponse(status_code=500, content={"detail": msg}) + raise InvalidReturnValueError(msg) from exc def model_to_dict(model: Optional[BaseModel]) -> Dict[str, Any]: diff --git a/tests/test_action_manager.py b/tests/test_action_manager.py index 37242d42..1910bd33 100644 --- a/tests/test_action_manager.py +++ b/tests/test_action_manager.py @@ -74,4 +74,7 @@ def test_actions_list(client): r2 = client.get(ACTION_INVOCATIONS_PATH) r2.raise_for_status() invocations = r2.json() + # Some keys aren't present in the list for performance/safety reasons + for k in ["input", "output", "log", "links"]: + del invocation[k] assert invocations == [invocation] diff --git a/tests/test_actions.py b/tests/test_actions.py index 16425014..a212d018 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -355,20 +355,19 @@ def make_unjsonable_any(self) -> Any: with server.test_client() as client: tc = lt.ThingClient.from_url("/naughty/", client=client) - # If a return type doesn't match the type hint, we get + # Here, the return type doesn't match the type hint, so it should fail + # to validate. with pytest.raises( (ServerActionError, FailedToInvokeActionError), - match=( - r"The return value from 'naughty.make_random_int' failed to validate " - r"against its output model." - ), + match="Error validating the output of 'naughty.make_random_int'", ): tc.make_random_int() - # If a return type is not JSONable + # Here, the type hint is vague so it validates OK, but it can't + # serialize. with pytest.raises( (ServerActionError, FailedToInvokeActionError), - match="Could not serialise invocation", + match="Error serializing invocation ", ) as excinfo: tc.make_unjsonable_any() assert "make_unjsonable_any" in str(excinfo) From ab66a12899ea71fe827ee38452db4caa688feb5f Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 14:52:02 +0100 Subject: [PATCH 10/18] Move links into summary invocation model. These may be used by some clients to find the full model, so they're useful here. --- src/labthings_fastapi/actions.py | 14 +++++++------- src/labthings_fastapi/invocations.py | 2 +- tests/test_action_manager.py | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 2e7d0df9..78240ab7 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -233,6 +233,12 @@ def summary_model(self) -> InvocationSummary: :return: a `InvocationSummary` representing this `Invocation`. """ + links = [ + LinkElement(rel="self", href=URLFor("action_invocation", id=self.id)), + LinkElement( + rel="output", href=URLFor("action_invocation_output", id=self.id) + ), + ] with self._status_lock: return InvocationSummary( status=self.status, @@ -242,6 +248,7 @@ def summary_model(self) -> InvocationSummary: timeStarted=self._start_time, timeCompleted=self._end_time, timeRequested=self._request_time, + links=links, ) def response(self) -> InvocationModel: @@ -253,12 +260,6 @@ def response(self) -> InvocationModel: :return: an `.InvocationModel` representing this `.Invocation`. """ - links = [ - LinkElement(rel="self", href=URLFor("action_invocation", id=self.id)), - LinkElement( - rel="output", href=URLFor("action_invocation_output", id=self.id) - ), - ] # The line below confuses MyPy because self.action **evaluates to** a Descriptor # object (i.e. we don't call __get__ on the descriptor). with self._status_lock: @@ -266,7 +267,6 @@ def response(self) -> InvocationModel: **dict(self.summary_model()), input=self.input, output=self.output_model_instance, - links=links, log=self.log, ) diff --git a/src/labthings_fastapi/invocations.py b/src/labthings_fastapi/invocations.py index e61e6125..c9512a1f 100644 --- a/src/labthings_fastapi/invocations.py +++ b/src/labthings_fastapi/invocations.py @@ -101,6 +101,7 @@ class InvocationSummary(BaseModel): timeStarted: Optional[datetime] timeRequested: Optional[datetime] timeCompleted: Optional[datetime] + links: Links = None InputT = TypeVar("InputT") @@ -119,7 +120,6 @@ class GenericInvocationModel(InvocationSummary, Generic[InputT, OutputT]): input: InputT output: OutputT log: Sequence[LogRecordModel] - links: Links = None InvocationModel = GenericInvocationModel[Any, Any] diff --git a/tests/test_action_manager.py b/tests/test_action_manager.py index 1910bd33..4b0603b3 100644 --- a/tests/test_action_manager.py +++ b/tests/test_action_manager.py @@ -75,6 +75,6 @@ def test_actions_list(client): r2.raise_for_status() invocations = r2.json() # Some keys aren't present in the list for performance/safety reasons - for k in ["input", "output", "log", "links"]: + for k in ["input", "output", "log"]: del invocation[k] assert invocations == [invocation] From f3cd449fc8e3d848be9539d219d5dc57a9d0111f Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 14:53:54 +0100 Subject: [PATCH 11/18] Improve testing of invalid values via HTTP This checks that we get the correct errors when accessing bad values over HTTP. Note that an invocation with an unserializable return value will raise an error if it is accessed individually, but won't cause the global endpoints to fail. --- tests/test_actions.py | 32 ++++++++++++++++++++++++++++ tests/test_properties.py | 46 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/tests/test_actions.py b/tests/test_actions.py index a212d018..4798f46a 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -363,6 +363,18 @@ def make_unjsonable_any(self) -> Any: ): tc.make_random_int() + # The action should still have run, so check that we can get the + # invocation. + actions = client.get("/naughty/make_random_int/").json() + assert len(actions) == 1 + invocation = client.get(actions[0]["href"]).json() + assert invocation["output"] is None + assert invocation["status"] == "error" + first_invocation_id = invocation["id"] + assert "Error validating the output" in invocation["log"][-1]["message"] + response = client.get(invocation["links"][1]["href"]) + assert response.status_code == 503 # There's no output as it failed. + # Here, the type hint is vague so it validates OK, but it can't # serialize. with pytest.raises( @@ -371,3 +383,23 @@ def make_unjsonable_any(self) -> Any: ) as excinfo: tc.make_unjsonable_any() assert "make_unjsonable_any" in str(excinfo) + + # Get the last invocation + actions = client.get("/naughty/make_unjsonable_any/").json() + + # The action should still have run, so check that we can get the + # invocation. + actions = client.get("/naughty/make_unjsonable_any/").json() + assert len(actions) == 1 + second_invocation_id = actions[0]["id"] + response = client.get(actions[0]["href"]) + assert response.status_code == 500 + assert "Error serializing" in response.json()["detail"] + # Try the direct link to the action's output + response = client.get(actions[0]["links"][1]["href"]) + assert response.status_code == 500 # The output won't serialize + assert "Error serializing" in response.json()["detail"] + + # Check the overall invocations endpoint isn't broken + actions = client.get("/action_invocations/").json() + assert {a["id"] for a in actions} == {first_invocation_id, second_invocation_id} diff --git a/tests/test_properties.py b/tests/test_properties.py index a1806b12..792d99ca 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -9,8 +9,10 @@ import labthings_fastapi as lt from labthings_fastapi.exceptions import ( + ClientPropertyError, NotBoundToInstanceError, ServerNotRunningError, + UnserializableTypeError, UnsupportedConstraintError, ) from labthings_fastapi.properties import BaseProperty, PropertyInfo @@ -18,6 +20,10 @@ from .temp_client import poll_task +class Unjsonable: + """A class that pydantic can't serialize.""" + + class PropertyTestThing(lt.Thing): """A Thing with various properties for testing.""" @@ -612,3 +618,43 @@ def test_title_and_description(name, title, description): description = title # If a description is present, ignore any trailing whitespace. assert (prop.description.rstrip() if prop.description else None) == description + + +def test_bad_type(): + """Test an obviously un-serializable type raises an error.""" + + class BrokenThing(lt.Thing): + broken: Unjsonable | None = lt.property(default=None) + + with pytest.raises(UnserializableTypeError, match="BrokenThing.broken"): + _ = BrokenThing.properties["broken"].model + + +def test_bad_values(): + """Ensure bad values in properties generate sensible HTTP errors.""" + + class BrokenThing(lt.Thing): + def __init__(self, **kwargs): + super().__init__(**kwargs) + # Set bad values here because we shouldn't have invalid defaults. + self.__dict__["intprop"] = 4.2 + self.__dict__["anyprop"] = Unjsonable() + + intprop: int = lt.property(default=0) + anyprop: Any = lt.property(default=None) + + server = lt.ThingServer.from_things({"broken": BrokenThing}) + with server.test_client() as client: + tc = lt.ThingClient.from_url("/broken/", client=client) + + # The first property won't validate + with pytest.raises( + ClientPropertyError, match="Error validating broken.intprop" + ): + _ = tc.intprop + + # The second property won't serialize + with pytest.raises( + ClientPropertyError, match="Error serializing broken.anyprop" + ): + _ = tc.anyprop From da24240adac2e99e4605c03609a1af26d55c1a6b Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 15:13:03 +0100 Subject: [PATCH 12/18] Remove a no-longer-needed type: ignore --- src/labthings_fastapi/actions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 78240ab7..0ae46b72 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -1022,7 +1022,7 @@ def start_action( @app.get( thing.path + self.name, - response_model=list[InvocationSummary], # type: ignore + response_model=list[InvocationSummary], # MyPy doesn't like the line above - but it works for FastAPI # to generate a response model. response_description=f"A list of every invocation of {self.name}.", From ad35b54cacfd62ca271bc53ceb6d56b95737555e Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 26 May 2026 10:20:13 +0100 Subject: [PATCH 13/18] Consistently spell "serialise" From @bprobert97's review --- src/labthings_fastapi/actions.py | 2 +- src/labthings_fastapi/exceptions.py | 6 +++--- src/labthings_fastapi/utilities/__init__.py | 12 ++++++------ tests/test_actions.py | 2 +- tests/test_properties.py | 18 +++++++++++++++--- 5 files changed, 26 insertions(+), 14 deletions(-) diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 0ae46b72..48257602 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -231,7 +231,7 @@ def cancel(self) -> None: def summary_model(self) -> InvocationSummary: """Generate a summary of the invocation suitable for HTTP. - :return: a `InvocationSummary` representing this `Invocation`. + :return: an `InvocationSummary` representing this `Invocation`. """ links = [ LinkElement(rel="self", href=URLFor("action_invocation", id=self.id)), diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 6dffe269..0efef708 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -268,7 +268,7 @@ def _append_to_args(self, message: str) -> None: # If there's only one string, assume it's a message and append self.args = (self.args[0] + "\n" + message,) else: - # If there are multiple arguments, add this as a further one + # If there are multiple or no arguments, add this as a further one self.args += (message,) def set_source_function(self, func: Callable) -> None: @@ -313,10 +313,10 @@ class InvalidReturnValueError(CausedByUserCodeError, RuntimeError): class UnserializableTypeError(CausedByUserCodeError, TypeError): - r"""A type has been specified that can't be serialized to JSON. + r"""A type has been specified that can't be serialised to JSON. This error generally means a property or action has a type that cannot be - serialized to JSON. This might be an instance of a custom class, or another + serialised to JSON. This might be an instance of a custom class, or another datatype that doesn't have a ready representation using JSON-compatible types. This error can often be fixed using `pydantic` annotations, or by using simple diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index 91a9937e..2265f853 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -149,7 +149,7 @@ def wrap_type( unsuitable type, for example `allow_inf_nan` for an `int` property, or any constraints for a `BaseModel` subclass. :raises UnserializableTypeError: if the type being wrapped is not able - to be serialized by `pydantic`\ . + to be serialised by `pydantic`\ . :raises RuntimeError: if other errors prevent Pydantic from creating a schema for the generated model. """ @@ -172,7 +172,7 @@ def wrap_type( ) except PydanticSchemaGenerationError as e: raise UnserializableTypeError( - f"LabThings does not know how to serialize {model!r} to JSON." + f"LabThings does not know how to serialise {model!r} to JSON." ) from e except RuntimeError as e: if "Unable to apply constraint" in str(e): @@ -283,10 +283,10 @@ def serialize_from_user_code( an endpoint function is typed as returning a `pydantic.BaseModel` instance. The validated model instance is serialised to JSON by calling ``model_dump_json()`` on the model instance, and the resulting string is returned - in a `Response` object. This uses `pydantic` serialization, written in Rust, + in a `Response` object. This uses `pydantic` serialisation, written in Rust, and outperforms the native `json` library significantly. - If the model can't be serialized, we raise an exception with information about + If the model can't be serialised, we raise an exception with information about the place in the user code where the problem occurred. :param model_instance: the `pydantic` model to use for validation. @@ -306,9 +306,9 @@ def serialize_from_user_code( ) except PydanticSerializationError as exc: msg = ( - f"Error serializing {description} to JSON.\n" + f"Error serialising {description} to JSON.\n" f"The value was validated as {repr(model_instance)}.\n" - f"The serialization error was '{exc}'.\n" + f"The serialisation error was '{exc}'.\n" f"{refer_to_user_code(code)}" ) raise InvalidReturnValueError(msg) from exc diff --git a/tests/test_actions.py b/tests/test_actions.py index 4798f46a..d8e606aa 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -376,7 +376,7 @@ def make_unjsonable_any(self) -> Any: assert response.status_code == 503 # There's no output as it failed. # Here, the type hint is vague so it validates OK, but it can't - # serialize. + # serialise. with pytest.raises( (ServerActionError, FailedToInvokeActionError), match="Error serializing invocation ", diff --git a/tests/test_properties.py b/tests/test_properties.py index 792d99ca..5f071928 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -21,7 +21,7 @@ class Unjsonable: - """A class that pydantic can't serialize.""" + """A class that pydantic can't serialise.""" class PropertyTestThing(lt.Thing): @@ -621,7 +621,19 @@ def test_title_and_description(name, title, description): def test_bad_type(): - """Test an obviously un-serializable type raises an error.""" + """Test an obviously un-serialisable type raises an error. + + Because type hints may be deferred, we don't attempt to build the model for a + property until it's needed. The ``broken`` property is typed as + ``Unjsonable | None`` and ``Unjsonable`` is not a class that `pydantic` can + serialise. We should therefore get an error when we attempt to build the model, + telling us that serialisation will fail. + + We could type the property as `typing.Any` which would not cause a problem + in this test, but would then fail later when we attempt to serialise the value. + + This is tested in `test_bad_values` below. + """ class BrokenThing(lt.Thing): broken: Unjsonable | None = lt.property(default=None) @@ -641,7 +653,7 @@ def __init__(self, **kwargs): self.__dict__["anyprop"] = Unjsonable() intprop: int = lt.property(default=0) - anyprop: Any = lt.property(default=None) + # The second property won't serialise server = lt.ThingServer.from_things({"broken": BrokenThing}) with server.test_client() as client: From 452f493e0bc74c69559437cf862c75f51ccb887e Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 26 May 2026 10:30:09 +0100 Subject: [PATCH 14/18] Additional search-and-replace for "serialize" -> "serialise" This codebase now consistently uses the `-ise` ending, --- docs/source/blobs.rst | 8 ++++---- src/labthings_fastapi/actions.py | 14 +++++++------- src/labthings_fastapi/exceptions.py | 2 +- src/labthings_fastapi/middleware/url_for.py | 2 +- src/labthings_fastapi/outputs/blob.py | 8 ++++---- src/labthings_fastapi/properties.py | 10 +++++----- src/labthings_fastapi/server/__init__.py | 4 ++-- src/labthings_fastapi/types/numpy.py | 4 ++-- src/labthings_fastapi/utilities/__init__.py | 8 ++++---- tests/test_actions.py | 8 ++++---- tests/test_properties.py | 15 ++++++++++----- tests/test_property.py | 2 +- 12 files changed, 45 insertions(+), 40 deletions(-) diff --git a/docs/source/blobs.rst b/docs/source/blobs.rst index 7b737930..d27c2b6d 100644 --- a/docs/source/blobs.rst +++ b/docs/source/blobs.rst @@ -143,12 +143,12 @@ On the client, we can use the `capture_image` action directly (as before), or we Currently, `.Blob` objects are only retained on the server as part of the output of the action that created them. This means they will expire after a timeout (by default, a few minutes). This mechanism is likely to change in future releases: the use of `.Blob` objects as inputs should be considered experimental. -HTTP interface and serialization +HTTP interface and serialisation -------------------------------- -`.Blob` objects can be serialized to JSON and deserialized from JSON. When this happens, the `.Blob` is represented as a JSON object with ``href`` and ``content_type`` fields. The ``href`` field is a link to the data. The ``content_type`` field is a string representing the MIME type of the data. It is worth noting that models may be nested: this means an action may return many `.Blob` objects in its output, either as a list or as fields in a `pydantic.BaseModel` subclass. Each `.Blob` in the output will be serialized to JSON with its URL and content type, and the client can then download the data from the URL, one download per `.Blob` object. +`.Blob` objects can be serialised to JSON and deserialised from JSON. When this happens, the `.Blob` is represented as a JSON object with ``href`` and ``content_type`` fields. The ``href`` field is a link to the data. The ``content_type`` field is a string representing the MIME type of the data. It is worth noting that models may be nested: this means an action may return many `.Blob` objects in its output, either as a list or as fields in a `pydantic.BaseModel` subclass. Each `.Blob` in the output will be serialised to JSON with its URL and content type, and the client can then download the data from the URL, one download per `.Blob` object. -When a `.Blob` is serialized, the URL is generated with a unique ID to allow it to be downloaded. The URL is not guaranteed to be permanent, and should not be used as a long-term reference to the data. For `.Blob` objects that are part of the output of an action, the URL will expire after 5 minutes (or the retention time set for the action), and the data will no longer be available for download after that time. +When a `.Blob` is serialised, the URL is generated with a unique ID to allow it to be downloaded. The URL is not guaranteed to be permanent, and should not be used as a long-term reference to the data. For `.Blob` objects that are part of the output of an action, the URL will expire after 5 minutes (or the retention time set for the action), and the data will no longer be available for download after that time. In order to run an action and download the data, currently an HTTP client must: @@ -168,7 +168,7 @@ Memory management and retention Management of `.Blob` objects is currently very basic: when a `.Blob` object is returned in the output of an Action that has been called via the HTTP interface, it will be retained as long as the action's output. This may be set on each action, and defaults to 5 minutes. This should be improved in the future to avoid memory management issues. -When a `.Blob` is serialized, a URL is generated with a unique ID to allow it to be downloaded. However, only a weak reference is held to the `.Blob`. Once an Action has finished running, the only strong reference to the `.Blob` should be held by the output property of the action invocation. The `.Blob` should be garbage collected once the output is no longer required, i.e. when the invocation is discarded - currently 5 minutes after the action completes, once the maximum number of invocations has been reached or when it is explicitly deleted by the client. +When a `.Blob` is serialised, a URL is generated with a unique ID to allow it to be downloaded. However, only a weak reference is held to the `.Blob`. Once an Action has finished running, the only strong reference to the `.Blob` should be held by the output property of the action invocation. The `.Blob` should be garbage collected once the output is no longer required, i.e. when the invocation is discarded - currently 5 minutes after the action completes, once the maximum number of invocations has been reached or when it is explicitly deleted by the client. The behaviour is different when actions are called from other actions. If `action_a` calls `action_b`, and `action_b` returns a `.Blob`, that `.Blob` will be subject to Python's usual garbage collection rules when `action_a` ends - i.e. it will not be retained unless it is included in the output of `action_a`. diff --git a/src/labthings_fastapi/actions.py b/src/labthings_fastapi/actions.py index 48257602..1ee5d35e 100644 --- a/src/labthings_fastapi/actions.py +++ b/src/labthings_fastapi/actions.py @@ -52,7 +52,7 @@ from .utilities import ( RootModelWrapper, model_to_dict, - serialize_from_user_code, + serialise_from_user_code, validate_from_user_code, ) from .invocations import InvocationSummary, InvocationModel, InvocationStatus @@ -62,7 +62,7 @@ InvocationCancelledError, InvocationError, NotConnectedToServerError, - UnserializableTypeError, + UnserialisableTypeError, ) from . import invocation_contexts from .utilities.introspection import ( @@ -505,7 +505,7 @@ def action_invocation(id: uuid.UUID) -> Response: """ try: invocation = self.get_invocation(id) - return serialize_from_user_code( + return serialise_from_user_code( model_instance=invocation.response(), description=f"invocation '{id}' of ", code=invocation.action.func, # type: ignore[attr-defined] @@ -569,7 +569,7 @@ def action_invocation_output(id: uuid.UUID) -> Response: # TODO: honour "accept" header return invocation.output.response() try: - return serialize_from_user_code( + return serialise_from_user_code( model_instance=invocation.output_model_instance, description=f"the output of {invocation}", code=invocation.action.func, @@ -753,7 +753,7 @@ def __init__( if more nuanced locking behaviour is required meaning the lock is acquired directly in the action code, for example using `~lt.ThingServerInterface.hold_global_lock`\ . - :raises UnserializableTypeError: if the return type of the action cannot + :raises UnserialisableTypeError: if the return type of the action cannot be serialised to JSON by `pydantic`\ . """ super().__init__() @@ -775,7 +775,7 @@ def __init__( self.output_model = RootModelWrapper.wrap_type( return_type(func), name=f"{name.title()}Output" ) - except UnserializableTypeError as e: + except UnserialisableTypeError as e: e.set_source_function(func) raise self.invocation_model = create_model( @@ -961,7 +961,7 @@ def start_action( ) background_tasks.add_task(action_manager.expire_invocations) try: - return serialize_from_user_code( + return serialise_from_user_code( model_instance=invocation.response(), description=f"{invocation}", status_code=201, diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 0efef708..200625f1 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -312,7 +312,7 @@ class InvalidReturnValueError(CausedByUserCodeError, RuntimeError): """ -class UnserializableTypeError(CausedByUserCodeError, TypeError): +class UnserialisableTypeError(CausedByUserCodeError, TypeError): r"""A type has been specified that can't be serialised to JSON. This error generally means a property or action has a type that cannot be diff --git a/src/labthings_fastapi/middleware/url_for.py b/src/labthings_fastapi/middleware/url_for.py index f34f453c..672d6ac5 100644 --- a/src/labthings_fastapi/middleware/url_for.py +++ b/src/labthings_fastapi/middleware/url_for.py @@ -9,7 +9,7 @@ Under the hood, this module defines a `url_for` function that performs the conversion. This function may only be run in certain places in the code, as it relies on a context variable. As a rule of thumb, it's OK to call -`url_for` from a serializer of a `pydantic` model, but you should not call +`url_for` from a serialiser of a `pydantic` model, but you should not call it from within an Action or Property. There are several places in LabThings where we need to be able to include URLs diff --git a/src/labthings_fastapi/outputs/blob.py b/src/labthings_fastapi/outputs/blob.py index 390ef27e..2b358b35 100644 --- a/src/labthings_fastapi/outputs/blob.py +++ b/src/labthings_fastapi/outputs/blob.py @@ -558,7 +558,7 @@ class Blob: so it should only be serialised in a request handler function. This functionality is intended for use by LabThings library functions only. Validation and serialisation behaviour is described in the docstrings of - `.Blob._validate` and `.Blob._serialize`. + `.Blob._validate` and `.Blob._serialise`. """ media_type: str = "*/*" @@ -602,7 +602,7 @@ def __get_pydantic_core_schema__( We tell `pydantic` to base its handling of `Blob` on the `.BlobModel` schema, with custom validation and serialisation. Validation and serialisation behaviour is described in the docstrings - of `.Blob._validate` and `.Blob._serialize`. + of `.Blob._validate` and `.Blob._serialise`. The JSONSchema is generated for `.BlobModel` but is then refined in `__get_pydantic_json_schema__` to include the ``media_type`` @@ -616,7 +616,7 @@ def __get_pydantic_core_schema__( cls._validate, BlobModel.__pydantic_core_schema__, serialization=core_schema.wrap_serializer_function_ser_schema( - cls._serialize, + cls._serialise, is_field_serializer=False, info_arg=False, when_used="always", @@ -688,7 +688,7 @@ def _validate(cls, value: Any, handler: Callable[[Any], BlobModel]) -> Self: raise ValueError(f"Blob ID {id} wasn't found on this server.") from error @classmethod - def _serialize( + def _serialise( cls, obj: Self, handler: Callable[[BlobModel], Mapping[str, str]] ) -> Mapping[str, str]: """Serialise the Blob to a dictionary. diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index a51295ba..93048f39 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -83,7 +83,7 @@ class attribute. Documentation is in strings immediately following the from .utilities import ( RootModelWrapper, labthings_data, - serialize_from_user_code, + serialise_from_user_code, validate_from_user_code, ) from .utilities.introspection import return_type @@ -99,7 +99,7 @@ class attribute. Documentation is in strings immediately following the PropertyRedefinitionError, ReadOnlyPropertyError, MissingTypeError, - UnserializableTypeError, + UnserialisableTypeError, UnsupportedConstraintError, ) from .thing_class_settings import get_validate_properties_on_set @@ -467,7 +467,7 @@ def model(self) -> type[BaseModel]: subclass, this returns it unchanged. :return: a Pydantic model for the property's type. - :raises UnserializableTypeError: if the property can't be serialized + :raises UnserialisableTypeError: if the property can't be serialised by `pydantic` to JSON. """ if self._model is None: @@ -477,7 +477,7 @@ def model(self) -> type[BaseModel]: constraints=self.constraints, name=f"{self.name.title()}Value", ) - except UnserializableTypeError as e: + except UnserialisableTypeError as e: e.set_source_class(self.owning_class, self.name) raise return self._model @@ -577,7 +577,7 @@ def get_property() -> Response: description=f"{thing.name}.{self.name}", code=(self.owning_class, self.name), ) - return serialize_from_user_code( + return serialise_from_user_code( model_instance=instance, description=f"{thing.name}.{self.name}", code=(self.owning_class, self.name), diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 18871594..31ea99b5 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -253,11 +253,11 @@ async def global_lock_exception_handler( ) @self.app.exception_handler(PydanticSerializationError) - async def serialization_error_handler( + async def serialisation_error_handler( request: Request, exc: PydanticSerializationError ) -> JSONResponse: LOGGER.error( - f"Couldn't serialize response to {request.url} because of error: \n" + f"Couldn't serialise response to {request.url} because of error: \n" f"{exc}" ) return JSONResponse(status_code=500, content={"detail": str(exc)}) diff --git a/src/labthings_fastapi/types/numpy.py b/src/labthings_fastapi/types/numpy.py index 2fb27db0..b78e1722 100644 --- a/src/labthings_fastapi/types/numpy.py +++ b/src/labthings_fastapi/types/numpy.py @@ -126,7 +126,7 @@ def denumpify(v: Any) -> Any: return v -def denumpify_serializer(v: Any, nxt: SerializerFunctionWrapHandler) -> Any: +def denumpify_serialiser(v: Any, nxt: SerializerFunctionWrapHandler) -> Any: """Denumpify mappings before serialization. This is intended for use as a "wrap serialiser" in `pydantic`, and @@ -145,5 +145,5 @@ def denumpify_serializer(v: Any, nxt: SerializerFunctionWrapHandler) -> Any: class DenumpifyingDict(RootModel): """A `pydantic` model for a dictionary that converts arrays to lists.""" - root: Annotated[Mapping, WrapSerializer(denumpify_serializer)] + root: Annotated[Mapping, WrapSerializer(denumpify_serialiser)] model_config = ConfigDict(arbitrary_types_allowed=True) diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index 2265f853..c04130a9 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -21,7 +21,7 @@ from labthings_fastapi.exceptions import ( InvalidReturnValueError, UnsupportedConstraintError, - UnserializableTypeError, + UnserialisableTypeError, ) from .introspection import EmptyObject @@ -148,7 +148,7 @@ def wrap_type( :raises UnsupportedConstraintError: if constraints are provided for an unsuitable type, for example `allow_inf_nan` for an `int` property, or any constraints for a `BaseModel` subclass. - :raises UnserializableTypeError: if the type being wrapped is not able + :raises UnserialisableTypeError: if the type being wrapped is not able to be serialised by `pydantic`\ . :raises RuntimeError: if other errors prevent Pydantic from creating a schema for the generated model. @@ -171,7 +171,7 @@ def wrap_type( __base__=cls, ) except PydanticSchemaGenerationError as e: - raise UnserializableTypeError( + raise UnserialisableTypeError( f"LabThings does not know how to serialise {model!r} to JSON." ) from e except RuntimeError as e: @@ -271,7 +271,7 @@ def validate_from_user_code( raise InvalidReturnValueError(msg) from e -def serialize_from_user_code( +def serialise_from_user_code( model_instance: BaseModel, description: str, status_code: int = 200, diff --git a/tests/test_actions.py b/tests/test_actions.py index d8e606aa..2471ac3f 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -379,7 +379,7 @@ def make_unjsonable_any(self) -> Any: # serialise. with pytest.raises( (ServerActionError, FailedToInvokeActionError), - match="Error serializing invocation ", + match="Error serialising invocation ", ) as excinfo: tc.make_unjsonable_any() assert "make_unjsonable_any" in str(excinfo) @@ -394,11 +394,11 @@ def make_unjsonable_any(self) -> Any: second_invocation_id = actions[0]["id"] response = client.get(actions[0]["href"]) assert response.status_code == 500 - assert "Error serializing" in response.json()["detail"] + assert "Error serialising" in response.json()["detail"] # Try the direct link to the action's output response = client.get(actions[0]["links"][1]["href"]) - assert response.status_code == 500 # The output won't serialize - assert "Error serializing" in response.json()["detail"] + assert response.status_code == 500 # The output won't serialise + assert "Error serialising" in response.json()["detail"] # Check the overall invocations endpoint isn't broken actions = client.get("/action_invocations/").json() diff --git a/tests/test_properties.py b/tests/test_properties.py index 5f071928..9f0da758 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -12,7 +12,7 @@ ClientPropertyError, NotBoundToInstanceError, ServerNotRunningError, - UnserializableTypeError, + UnserialisableTypeError, UnsupportedConstraintError, ) from labthings_fastapi.properties import BaseProperty, PropertyInfo @@ -638,7 +638,7 @@ def test_bad_type(): class BrokenThing(lt.Thing): broken: Unjsonable | None = lt.property(default=None) - with pytest.raises(UnserializableTypeError, match="BrokenThing.broken"): + with pytest.raises(UnserialisableTypeError, match="BrokenThing.broken"): _ = BrokenThing.properties["broken"].model @@ -652,8 +652,13 @@ def __init__(self, **kwargs): self.__dict__["intprop"] = 4.2 self.__dict__["anyprop"] = Unjsonable() + # The first property won't validate it's initial value of 4.2 + # This will result in a validation error, which should be handled. intprop: int = lt.property(default=0) - # The second property won't serialise + + # The second property won't serialise, but will validate as it's + # typed as `Any`. + anyprop: Any = lt.property(default=None) server = lt.ThingServer.from_things({"broken": BrokenThing}) with server.test_client() as client: @@ -665,8 +670,8 @@ def __init__(self, **kwargs): ): _ = tc.intprop - # The second property won't serialize + # The second property won't serialise with pytest.raises( - ClientPropertyError, match="Error serializing broken.anyprop" + ClientPropertyError, match="Error serialising broken.anyprop" ): _ = tc.anyprop diff --git a/tests/test_property.py b/tests/test_property.py index a778700c..cb19274e 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -418,7 +418,7 @@ class BadIntModel(pydantic.BaseModel): bad_m = MyModel.model_construct(a="not an int", b=6) assert modelprop.validate(bad_m) is bad_m with warnings.catch_warnings(): - warnings.filterwarnings("ignore", "Pydantic serializer warnings") + warnings.filterwarnings("ignore", "Pydantic serialiser warnings") # Check that passing the same data in as a dict fails validation.# with pytest.raises(pydantic.ValidationError): modelprop.validate(bad_m.model_dump()) From 09398573e13622628adf3a99a7a761fe46571747 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 26 May 2026 21:22:50 +0100 Subject: [PATCH 15/18] Simplify and test exceptions There's now code in the exceptions module that needs testing: this is now done. The code is also simplified to remove branching, using one form that works regardless of how many arguments there are. --- src/labthings_fastapi/exceptions.py | 18 +++++------ tests/test_exceptions.py | 48 +++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 11 deletions(-) create mode 100644 tests/test_exceptions.py diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 200625f1..604b252b 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -262,14 +262,11 @@ class CausedByUserCodeError(Exception): def _append_to_args(self, message: str) -> None: """Add a message to the exception's arguments. + The message will be appended to the first (and usually only) argument. + :param message: the message to append. """ - if len(self.args) == 1: - # If there's only one string, assume it's a message and append - self.args = (self.args[0] + "\n" + message,) - else: - # If there are multiple or no arguments, add this as a further one - self.args += (message,) + self.args = (self.args[0] + "\n" + message,) + self.args[1:] def set_source_function(self, func: Callable) -> None: """Add the location of a user-supplied function to the error message. @@ -288,11 +285,10 @@ def set_source_class(self, cls: type, attr: str | None = None) -> None: :param cls: the class that caused this error. :param attr: the attribute name that caused this error. """ - self._append_to_args( - f"This was likely caused by '{cls.__module__}.{cls.__qualname__}.{attr}" - if attr - else "'." - ) + name = f"{cls.__module__}.{cls.__qualname__}" + if attr: + name += f".{attr}" + self._append_to_args(f"\nThis was likely caused by '{name}'.") class InvalidReturnValueError(CausedByUserCodeError, RuntimeError): diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py new file mode 100644 index 00000000..0c138a66 --- /dev/null +++ b/tests/test_exceptions.py @@ -0,0 +1,48 @@ +"""Test code for exceptions. + +There's not much code to test in the exceptions module: currently just the logic that +adds user code details to certain errors. +""" + +import pytest + +from labthings_fastapi import exceptions + + +EXCEPTIONS = ( + exceptions.InvalidReturnValueError, + exceptions.UnserialisableTypeError, + exceptions.CausedByUserCodeError, +) + + +def example_function(): + """An example function.""" + + +class ExampleClass: + """An example class with a property.""" + + prop: int = 0 + + +@pytest.mark.parametrize("cls", EXCEPTIONS) +@pytest.mark.parametrize("args", [("message",), ("arg_1", "arg_2")]) +def test_appending_exception_data(cls, args): + """Check that we can append user code to an exception.""" + exc: exceptions.CausedByUserCodeError = cls(*args) + exc.set_source_class(ExampleClass) + assert "test_exceptions.ExampleClass" in str(exc) + + exc: exceptions.CausedByUserCodeError = cls(*args) + exc.set_source_class(ExampleClass, "prop") + assert "test_exceptions.ExampleClass.prop" in str(exc) + + exc: exceptions.CausedByUserCodeError = cls(*args) + exc.set_source_function(example_function) + file = example_function.__code__.co_filename + line = example_function.__code__.co_firstlineno + assert file.endswith("test_exceptions.py") + assert isinstance(line, int) + assert f"{file}:{line}" in str(exc) + assert "example_function" in str(exc) From bb0cd56ee6fb869b3d11016470a8ed72bf0ec884 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 26 May 2026 22:44:41 +0100 Subject: [PATCH 16/18] Remove a defunct error check There's now a test for the initialiser of FunctionalProperty. This revealed that there was a spurious check for missing return types: we already substitute `Any` in this case. I also fixed a format issue with a type hint that was getting split over multiple lines. --- src/labthings_fastapi/properties.py | 19 +------------------ tests/test_property.py | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 93048f39..525e2a26 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -98,7 +98,6 @@ class attribute. Documentation is in strings immediately following the NotConnectedToServerError, PropertyRedefinitionError, ReadOnlyPropertyError, - MissingTypeError, UnserialisableTypeError, UnsupportedConstraintError, ) @@ -910,8 +909,6 @@ def __init__( :param use_global_lock: may be set to `False` to disable the global lock for setting this property. By default, if global locking is enabled, we hold the global lock while setting the property. - - :raises MissingTypeError: if the getter does not have a return type annotation. """ super().__init__(constraints=constraints, use_global_lock=use_global_lock) self._fget = fget @@ -920,23 +917,9 @@ def __init__( # If there is a docstring on the getter, use it as the property's docstring. # BaseDescriptor parses __doc__ to generate the title and description. self.__doc__ = fget.__doc__ - if self._type is None: - msg = ( - f"{fget} does not have a valid type. " - "Return type annotations are required for property getters." - ) - raise MissingTypeError(msg) self._fset: Callable[[Owner, Value], None] | None = None # setter function # `_freset` should reset the property to its default value. - self._freset: ( - Callable[ - [ - Owner, - ], - None, - ] - | None - ) = None + self._freset: Callable[[Owner], None] | None = None # `_default_factory` should return a default value. self._default_factory: Callable[[], Value] | None = None self.readonly: bool = True diff --git a/tests/test_property.py b/tests/test_property.py index cb19274e..ed12594b 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -314,6 +314,32 @@ def prop(self) -> bool: Example.prop.property_affordance(example, None) +def test_functionalproperty_initialisation(): + """Tests for `FunctionalProperty.__init__` including errors.""" + + def intgetter(self: lt.Thing) -> int: + """An integer getter with a docstring.""" + pass + + # Check we can make a property, and that it picks up type and docstring + class Example: + prop = FunctionalProperty(intgetter) + + assert Example.prop.value_type is int + assert Example.prop.__doc__ and "integer getter" in Example.prop.__doc__ + + def untyped(self: lt.Thing): + """A function without a return type.""" + + # Check that missing type annotations are `Any` + class Example2: + prop = FunctionalProperty(untyped) + + assert Example2.prop._type is Any + assert Example2.prop.value_type is Any + assert "Any" in str(Example2.prop.model) + + def test_propertyinfo(mocker): """Test out the PropertyInfo class.""" From d7dc0a4a6e8279a9e1f7d7e1bde0f5abeb326a8b Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 26 May 2026 22:55:25 +0100 Subject: [PATCH 17/18] Improve test of failing action There's a concurrency issue that means sometimes an action failed to invoke, and other times it invoked but failed to complete - that gave two slightly different errors. The test should now pass for either. --- tests/test_actions.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_actions.py b/tests/test_actions.py index 2471ac3f..69592448 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -379,10 +379,12 @@ def make_unjsonable_any(self) -> Any: # serialise. with pytest.raises( (ServerActionError, FailedToInvokeActionError), - match="Error serialising invocation ", + match="Error serialising ", ) as excinfo: tc.make_unjsonable_any() assert "make_unjsonable_any" in str(excinfo) + func = NaughtyThing.make_unjsonable_any.func.__code__ + assert f"{func.co_filename}:{func.co_firstlineno}" in str(excinfo) # Get the last invocation actions = client.get("/naughty/make_unjsonable_any/").json() From 9edffb39043221fcf52b2971e93aaed9d93313b0 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 26 May 2026 23:18:36 +0100 Subject: [PATCH 18/18] Add tests for error handling in poll_invocation. The error handling code in poll_invocation now has a couple of explicit tests, which make sure it does the right thing if the error is not in the expected format. --- tests/test_thing_client.py | 35 ++++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/tests/test_thing_client.py b/tests/test_thing_client.py index c170535a..460caebf 100644 --- a/tests/test_thing_client.py +++ b/tests/test_thing_client.py @@ -2,10 +2,16 @@ import re +import httpx import pytest import labthings_fastapi as lt -from labthings_fastapi.exceptions import ClientPropertyError, FailedToInvokeActionError +from labthings_fastapi.client import poll_invocation +from labthings_fastapi.exceptions import ( + ClientPropertyError, + FailedToInvokeActionError, + ServerActionError, +) class ThingToTest(lt.Thing): @@ -271,3 +277,30 @@ def test_call_that_errors(thing_client): r'File ".*test_thing_client\.py", line \d+, in throw_value_error', full_message, ) + + +def test_poll_invocation_errors(mocker): + """Test for errors that may occur when polling a task.""" + invocation = { + "links": [{"href": "http://fake/", "rel": "self"}], + "status": "running", + "action": "example_action_name", + "id": 0, + } + # This error conforms to "problem_details" format + client = mocker.Mock(spec=httpx.Client) + response = mocker.Mock(spec=httpx.Response) + response.is_error = True + response.status_code = 500 + response.json = mocker.MagicMock(return_value={"detail": "expected error text"}) + client.get = mocker.MagicMock(return_value=response) + with pytest.raises(ServerActionError, match="expected error text") as excinfo: + poll_invocation(client, invocation) + assert "detail" not in str(excinfo) + + # An error not conforming to that format should be dumped as text + response.json = mocker.MagicMock(return_value={}) + response.text = "direct from response.text" + with pytest.raises(ServerActionError, match="direct from response.text") as excinfo: + poll_invocation(client, invocation) + assert "detail" not in str(excinfo)