From 5637d5b93fd54deed0b1684d3696d053f26e2a43 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 12:23:11 +0100 Subject: [PATCH 01/12] 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 a5014a1afc0541189e39a1796343d96914f41a47 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 12:32:38 +0100 Subject: [PATCH 02/12] 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 d693f26728c13fbce1f0f77ccb0af3c15e968a11 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 6 May 2026 14:02:12 +0100 Subject: [PATCH 03/12] 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 4b46e4d8..feaafc53 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 1a60cdd88c7985416c42e0acea3d94233ba96e68 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 7 May 2026 09:29:00 +0100 Subject: [PATCH 04/12] 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 3e4a7625d56ed60fa65c24d35b4ed8e0d6da7a27 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 11 May 2026 20:59:50 +0100 Subject: [PATCH 05/12] 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 cd734f388861911ce366d4a0c81dc71ef5e4acef Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 11 May 2026 23:23:21 +0100 Subject: [PATCH 06/12] 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 be758667b34bcbc8016df0a1d013899333518881 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 11:24:58 +0100 Subject: [PATCH 07/12] 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 feaafc53..4e7d502b 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 fc0ac47d79e8db6c435aec1bafe031824671950c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 11:27:28 +0100 Subject: [PATCH 08/12] 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 2122b871a7c465ddaf87faf22738af6bc62843d3 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 14:00:46 +0100 Subject: [PATCH 09/12] 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 48591b0567effc3e11fb0596ea858480b3a08f75 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 14:52:02 +0100 Subject: [PATCH 10/12] 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 31f18c764e140de30378faf00a0b977aff7dabee Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 14:53:54 +0100 Subject: [PATCH 11/12] 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 55f5a216aca33a32e0c0f7cfc1685c55f03305aa Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 13 May 2026 15:13:03 +0100 Subject: [PATCH 12/12] 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}.",