From 431f7acba205812453c3c4b2a3588f6be9838d1a Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 01/48] Increase strictness of mypy --- pyproject.toml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index d1f0347..7b002e3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -108,7 +108,12 @@ property-decorators = [ ] [tool.mypy] +files = ["src/", "typing_tests/"] plugins = ["pydantic.mypy"] +disallow_untyped_defs = true +check_untyped_defs = true +disallow_untyped_decorators = true +warn_unused_ignores = true [tool.flake8] From 024847f57dd63867586aa807f1c4d9115718efb6 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 02/48] Add annotations to property I've added a stub __set__ to BaseProperty to improve type checking. Other than that, no code changes. --- src/labthings_fastapi/properties.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 9af1bbf..a88cbe4 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -381,7 +381,7 @@ def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: # the function to the decorator. if not self.readonly: - def set_property(body): # We'll annotate body later + def set_property(body: Any) -> None: # We'll annotate body later if isinstance(body, RootModel): body = body.root return self.__set__(thing, body) @@ -402,7 +402,7 @@ def set_property(body): # We'll annotate body later summary=self.title, description=f"## {self.title}\n\n{self.description or ''}", ) - def get_property(): + def get_property() -> Any: return self.__get__(thing) def property_affordance( @@ -445,6 +445,19 @@ def property_affordance( } ) + def __set__(self, obj: Thing, value: Any) -> None: + """Set the property (stub method). + + This is a stub ``__set__`` method to mark this as a data descriptor. + + :param obj: The Thing on which we are setting the value. + :param value: The new value for the Thing. + :raises NotImplementedError: as this must be overridden by concrete classes. + """ + raise NotImplementedError( + "__set__ must be overridden by property implementations." + ) + class DataProperty(BaseProperty[Value], Generic[Value]): """A Property descriptor that acts like a regular variable. @@ -602,7 +615,7 @@ def __set__( if emit_changed_event: self.emit_changed_event(obj, value) - def _observers_set(self, obj: Thing): + def _observers_set(self, obj: Thing) -> WeakSet: """Return the observers of this property. Each observer in this set will be notified when the property is changed. @@ -646,7 +659,7 @@ def emit_changed_event(self, obj: Thing, value: Value) -> None: value, ) - async def emit_changed_event_async(self, obj: Thing, value: Value): + async def emit_changed_event_async(self, obj: Thing, value: Value) -> None: """Notify subscribers that the property has changed. This function may only be run in the `anyio` event loop. See @@ -793,7 +806,7 @@ def instance_get(self, obj: Thing) -> Value: """ return self.fget(obj) - def __set__(self, obj: Thing, value: Value): + def __set__(self, obj: Thing, value: Value) -> None: """Set the value of the property. :param obj: the `.Thing` on which the attribute is accessed. From 2978b393839f20fec62427d39cd74649727f62a7 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 03/48] Add type annotations to actions The ActionDescriptor really confuses mypy. It would be nice to find a better way to reference actions. --- src/labthings_fastapi/actions/__init__.py | 42 ++++++++++++++--------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/labthings_fastapi/actions/__init__.py b/src/labthings_fastapi/actions/__init__.py index 81e5c73..e278fab 100644 --- a/src/labthings_fastapi/actions/__init__.py +++ b/src/labthings_fastapi/actions/__init__.py @@ -168,7 +168,7 @@ def status(self) -> InvocationStatus: return self._status @property - def action(self): + def action(self) -> ActionDescriptor: """The `.ActionDescriptor` object running in this thread.""" action = self.action_ref() assert action is not None, "The action for an `Invocation` has been deleted!" @@ -210,10 +210,12 @@ def response(self, request: Optional[Request] = None) -> InvocationModel: LinkElement(rel="self", href=href), LinkElement(rel="output", href=href + "/output"), ] - return self.action.invocation_model( + # 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[call-overload] status=self.status, id=self.id, - action=self.thing.path + self.action.name, + action=self.thing.path + self.action.name, # type: ignore[call-overload] href=href, timeStarted=self._start_time, timeCompleted=self._end_time, @@ -249,24 +251,25 @@ def run(self) -> None: See `.Invocation.status` for status values. """ + # self.action evaluates to an ActionDescriptor. This confuses mypy, + # which thinks we are calling ActionDescriptor.__get__. + action: ActionDescriptor = self.action # type: ignore[call-overload] try: - self.action.emit_changed_event(self.thing, self._status) + action.emit_changed_event(self.thing, str(self._status)) # Capture just this thread's log messages handler = DequeLogHandler(dest=self._log) logger = invocation_logger(self.id) logger.addHandler(handler) - action = self.action thing = self.thing kwargs = model_to_dict(self.input) - assert action is not None assert thing is not None with self._status_lock: self._status = InvocationStatus.RUNNING self._start_time = datetime.datetime.now() - self.action.emit_changed_event(self.thing, self._status) + action.emit_changed_event(self.thing, str(self._status)) # The next line actually runs the action. ret = action.__get__(thing)(**kwargs, **self.dependencies) @@ -274,12 +277,12 @@ def run(self) -> None: with self._status_lock: self._return_value = ret self._status = InvocationStatus.COMPLETED - self.action.emit_changed_event(self.thing, self._status) + action.emit_changed_event(self.thing, str(self._status)) except InvocationCancelledError: logger.info(f"Invocation {self.id} was cancelled.") with self._status_lock: self._status = InvocationStatus.CANCELLED - self.action.emit_changed_event(self.thing, self._status) + action.emit_changed_event(self.thing, str(self._status)) except Exception as e: # skipcq: PYL-W0703 # First log if isinstance(e, InvocationError): @@ -291,7 +294,7 @@ def run(self) -> None: with self._status_lock: self._status = InvocationStatus.ERROR self._exception = e - self.action.emit_changed_event(self.thing, self._status) + action.emit_changed_event(self.thing, str(self._status)) finally: with self._status_lock: self._end_time = datetime.datetime.now() @@ -341,9 +344,9 @@ def emit(self, record: logging.LogRecord) -> None: class ActionManager: """A class to manage a collection of actions.""" - def __init__(self): + def __init__(self) -> None: """Set up an `.ActionManager`.""" - self._invocations = {} + self._invocations: dict[uuid.UUID, Invocation] = {} self._invocations_lock = Lock() @property @@ -443,10 +446,13 @@ def list_invocations( i.response(request=request) for i in self.invocations if thing is None or i.thing == thing - if action is None or i.action == action + if action is None or i.action == action # type: ignore[call-overload] + # i.action evaluates to an ActionDescriptor, which confuses mypy - it + # thinks we are calling ActionDescriptor.__get__ but this isn't ever + # called. ] - def expire_invocations(self): + def expire_invocations(self) -> None: """Delete invocations that have passed their expiry time.""" to_delete = [] with self._invocations_lock: @@ -465,7 +471,9 @@ def attach_to_app(self, app: FastAPI) -> None: """ @app.get(ACTION_INVOCATIONS_PATH, response_model=list[InvocationModel]) - def list_all_invocations(request: Request, _blob_manager: BlobIOContextDep): + def list_all_invocations( + request: Request, _blob_manager: BlobIOContextDep + ) -> list[InvocationModel]: return self.list_invocations(request=request) @app.get( @@ -512,7 +520,9 @@ def action_invocation( 503: {"description": "No result is available for this invocation"}, }, ) - def action_invocation_output(id: uuid.UUID, _blob_manager: BlobIOContextDep): + def action_invocation_output( + id: uuid.UUID, _blob_manager: BlobIOContextDep + ) -> Any: """Get the output of an action invocation. This returns just the "output" component of the action invocation. If the From 317359860c06dfc57643625beb0419f2099b0894 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 04/48] Add type annotations to client module. --- src/labthings_fastapi/client/__init__.py | 25 ++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/labthings_fastapi/client/__init__.py b/src/labthings_fastapi/client/__init__.py index 320c9fd..0aeaee5 100644 --- a/src/labthings_fastapi/client/__init__.py +++ b/src/labthings_fastapi/client/__init__.py @@ -148,7 +148,7 @@ def get_property(self, path: str) -> Any: r.raise_for_status() return r.json() - def set_property(self, path: str, value: Any): + def set_property(self, path: str, value: Any) -> None: """Make a PUT request to set the value of a property. :param path: the URI of the ``getproperty`` endpoint, relative @@ -159,7 +159,7 @@ def set_property(self, path: str, value: Any): r = self.client.put(urljoin(self.path, path), json=value) r.raise_for_status() - def invoke_action(self, path: str, **kwargs): + def invoke_action(self, path: str, **kwargs: Any) -> Any: r"""Invoke an action on the Thing. This method will make the initial POST request to invoke an action, @@ -180,8 +180,9 @@ def invoke_action(self, path: str, **kwargs): :raise RuntimeError: is raised if the action does not complete successfully. """ for k in kwargs.keys(): - if isinstance(kwargs[k], ClientBlobOutput): - kwargs[k] = {"href": kwargs[k].href, "media_type": kwargs[k].media_type} + value = kwargs[k] + if isinstance(value, ClientBlobOutput): + kwargs[k] = {"href": value.href, "media_type": value.media_type} r = self.client.post(urljoin(self.path, path), json=kwargs) r.raise_for_status() invocation = poll_invocation(self.client, r.json()) @@ -270,7 +271,9 @@ class Client(cls): # type: ignore[valid-type, misc] class PropertyClientDescriptor: """A base class for properties on `.ThingClient` objects.""" - pass + name: str + type: type | BaseModel + path: str def property_descriptor( @@ -312,10 +315,10 @@ class P(PropertyClientDescriptor): if readable: def __get__( - self, + self: PropertyClientDescriptor, obj: Optional[ThingClient] = None, _objtype: Optional[type[ThingClient]] = None, - ): + ) -> Any: if obj is None: return self return obj.get_property(self.name) @@ -324,7 +327,9 @@ def __get__( P.__get__ = __get__ # type: ignore[attr-defined] if writeable: - def __set__(self, obj: ThingClient, value: Any): + def __set__( + self: PropertyClientDescriptor, obj: ThingClient, value: Any + ) -> None: obj.set_property(self.name, value) __set__.__annotations__["value"] = model @@ -349,7 +354,7 @@ def add_action(cls: type[ThingClient], action_name: str, action: dict) -> None: format. """ - def action_method(self, **kwargs): + def action_method(self: ThingClient, **kwargs: Any) -> Any: return self.invoke_action(action_name, **kwargs) if "output" in action and "type" in action["output"]: @@ -359,7 +364,7 @@ def action_method(self, **kwargs): setattr(cls, action_name, action_method) -def add_property(cls: type[ThingClient], property_name: str, property: dict): +def add_property(cls: type[ThingClient], property_name: str, property: dict) -> None: """Add a property to a ThingClient subclass. A descriptor will be added to the provided class that makes the From a09723504b9b82f499511acd8b5c0b95061813a4 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 05/48] Improved type annotations for DirectThingClient This doesn't improve the fact that the generated class can't be type checked. --- src/labthings_fastapi/client/in_server.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/labthings_fastapi/client/in_server.py b/src/labthings_fastapi/client/in_server.py index 7bcdfbe..af54b63 100644 --- a/src/labthings_fastapi/client/in_server.py +++ b/src/labthings_fastapi/client/in_server.py @@ -113,18 +113,22 @@ class P(PropertyClientDescriptor): path = property_path or property_name def __get__( - self, + self: PropertyClientDescriptor, obj: Optional[DirectThingClient] = None, _objtype: Optional[type[DirectThingClient]] = None, - ): + ) -> Any: if obj is None: return self return getattr(obj._wrapped_thing, self.name) - def __set__(self, obj: DirectThingClient, value: Any): + def __set__( + self: PropertyClientDescriptor, obj: DirectThingClient, value: Any + ) -> None: setattr(obj._wrapped_thing, self.name, value) - def set_readonly(self, obj: DirectThingClient, value: Any): + def set_readonly( + self: PropertyClientDescriptor, obj: DirectThingClient, value: Any + ) -> None: raise AttributeError("This property is read-only.") if readable: @@ -198,7 +202,7 @@ def add_action( """ @wraps(action.func) - def action_method(self, **kwargs): + def action_method(self: DirectThingClient, **kwargs: Any) -> Any: dependency_kwargs = { param.name: self._dependencies[param.name] for param in action.dependency_params From 73227f74f99abab6bd17c1f8983d77319062c031 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 06/48] Add type annotations to Invocation. --- src/labthings_fastapi/dependencies/invocation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/dependencies/invocation.py b/src/labthings_fastapi/dependencies/invocation.py index 9777df4..85b0bf6 100644 --- a/src/labthings_fastapi/dependencies/invocation.py +++ b/src/labthings_fastapi/dependencies/invocation.py @@ -146,7 +146,7 @@ def __init__(self, id: InvocationID): threading.Event.__init__(self) self.invocation_id = id - def raise_if_set(self): + def raise_if_set(self) -> None: """Raise an exception if the event is set. This is intended as a compact alternative to: @@ -161,7 +161,7 @@ def raise_if_set(self): if self.is_set(): raise InvocationCancelledError("The action was cancelled.") - def sleep(self, timeout: float): + def sleep(self, timeout: float) -> None: r"""Sleep for a given time in seconds, but raise an exception if cancelled. This function can be used in place of `time.sleep`. It will usually behave From 4b7891fee2707e6b7d67001d8982be30a4fd9e63 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 07/48] Add more type annotations. --- src/labthings_fastapi/client/in_server.py | 2 +- src/labthings_fastapi/decorators/__init__.py | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/labthings_fastapi/client/in_server.py b/src/labthings_fastapi/client/in_server.py index af54b63..9c3d6ff 100644 --- a/src/labthings_fastapi/client/in_server.py +++ b/src/labthings_fastapi/client/in_server.py @@ -277,7 +277,7 @@ def direct_thing_client_class( def init_proxy( self: DirectThingClient, request: Request, **dependencies: Mapping[str, Any] - ): + ) -> None: r"""Initialise a DirectThingClient (this docstring will be replaced). :param self: The DirectThingClient instance we're initialising. diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index b1950b7..f1c6146 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -37,7 +37,7 @@ """ from functools import wraps, partial -from typing import Optional, Callable, overload +from typing import Any, Optional, Callable, overload from ..descriptors import ( ActionDescriptor, EndpointDescriptor, @@ -45,7 +45,7 @@ ) -def mark_thing_action(func: Callable, **kwargs) -> ActionDescriptor: +def mark_thing_action(func: Callable, **kwargs: Any) -> ActionDescriptor: r"""Mark a method of a Thing as an Action. We replace the function with a descriptor that's a @@ -65,12 +65,12 @@ class ActionDescriptorSubclass(ActionDescriptor): @overload -def thing_action(func: Callable, **kwargs) -> ActionDescriptor: ... +def thing_action(func: Callable, **kwargs: Any) -> ActionDescriptor: ... @overload def thing_action( - **kwargs, + **kwargs: Any, ) -> Callable[ [ Callable, @@ -81,7 +81,7 @@ def thing_action( @wraps(mark_thing_action) def thing_action( - func: Optional[Callable] = None, **kwargs + func: Optional[Callable] = None, **kwargs: Any ) -> ( ActionDescriptor | Callable[ @@ -121,7 +121,7 @@ def thing_action( def fastapi_endpoint( - method: HTTPMethod, path: Optional[str] = None, **kwargs + method: HTTPMethod, path: Optional[str] = None, **kwargs: Any ) -> Callable[[Callable], EndpointDescriptor]: r"""Mark a function as a FastAPI endpoint without making it an action. From bbed6482761e28188eb45c5b426a021d20a52e1c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 08/48] Type annotations on Action and Endpoint --- src/labthings_fastapi/descriptors/action.py | 22 ++++++++++--------- src/labthings_fastapi/descriptors/endpoint.py | 20 +++++++++-------- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/labthings_fastapi/descriptors/action.py b/src/labthings_fastapi/descriptors/action.py index 68a409a..7c06d89 100644 --- a/src/labthings_fastapi/descriptors/action.py +++ b/src/labthings_fastapi/descriptors/action.py @@ -122,11 +122,11 @@ def __init__( self.invocation_model.__name__ = f"{self.name}_invocation" @overload - def __get__(self, obj: Literal[None], type=None) -> ActionDescriptor: # noqa: D105 + def __get__(self, obj: Literal[None], type: type[Thing]) -> ActionDescriptor: # noqa: D105 ... @overload - def __get__(self, obj: Thing, type=None) -> Callable: # noqa: D105 + def __get__(self, obj: Thing, type: type[Thing] | None = None) -> Callable: # noqa: D105 ... def __get__( @@ -158,17 +158,17 @@ def __get__( return partial(self.func, obj) @property - def name(self): + def name(self) -> str: """The name of the wrapped function.""" return self.func.__name__ @property - def title(self): + def title(self) -> str: """A human-readable title.""" return get_summary(self.func) or self.name @property - def description(self): + def description(self) -> str | None: """A description of the action.""" return get_docstring(self.func, remove_summary=True) @@ -189,7 +189,7 @@ def _observers_set(self, obj: Thing) -> WeakSet: ld.action_observers[self.name] = WeakSet() return ld.action_observers[self.name] - def emit_changed_event(self, obj: Thing, status: str): + def emit_changed_event(self, obj: Thing, status: str) -> None: """Notify subscribers that the action status has changed. This function is run from within the `.Invocation` thread that @@ -266,12 +266,12 @@ def start_action( action_manager: ActionManagerContextDep, _blob_manager: BlobIOContextDep, request: Request, - body, + body: Any, # This annotation will be overwritten below. id: InvocationID, cancel_hook: CancelHook, background_tasks: BackgroundTasks, - **dependencies, - ): + **dependencies: Any, + ) -> InvocationModel: action = action_manager.invoke_action( action=self, thing=thing, @@ -318,6 +318,7 @@ def start_action( if hasattr(self.output_model, "media_type"): responses[200]["content"][self.output_model.media_type] = {} # Now we can add the endpoint to the app. + assert thing.path is not None, "Can't add the endpoint without thing.path!" app.post( thing.path + self.name, response_model=self.invocation_model, @@ -341,7 +342,7 @@ def start_action( ) def list_invocations( action_manager: ActionManagerContextDep, _blob_manager: BlobIOContextDep - ): + ) -> list[InvocationModel]: return action_manager.list_invocations(self, thing) def action_affordance( @@ -359,6 +360,7 @@ def action_affordance( :return: An `.ActionAffordance` describing this action. """ path = path or thing.path + assert path is not None, "Can't generate forms without a path!" forms = [ Form[ActionOp](href=path + self.name, op=[ActionOp.invokeaction]), ] diff --git a/src/labthings_fastapi/descriptors/endpoint.py b/src/labthings_fastapi/descriptors/endpoint.py index 6beec6a..47c542c 100644 --- a/src/labthings_fastapi/descriptors/endpoint.py +++ b/src/labthings_fastapi/descriptors/endpoint.py @@ -19,6 +19,7 @@ from ..utilities.introspection import get_docstring, get_summary from typing import ( + Any, Callable, Literal, Mapping, @@ -45,7 +46,7 @@ def __init__( func: Callable, http_method: HTTPMethod = "get", path: Optional[str] = None, - **kwargs: Mapping, + **kwargs: Mapping[str, Any], ): r"""Initialise an EndpointDescriptor. @@ -67,10 +68,10 @@ def __init__( self.kwargs = kwargs @overload - def __get__(self, obj: Literal[None], type=None) -> Self: ... # noqa: D105 + def __get__(self, obj: Literal[None], type: type[Thing] | None = None) -> Self: ... # noqa: D105 @overload - def __get__(self, obj: Thing, type=None) -> Callable: ... # noqa: D105 + def __get__(self, obj: Thing, type: type[Thing] | None = None) -> Callable: ... # noqa: D105 def __get__( self, obj: Optional[Thing], type: type[Thing] | None = None @@ -96,26 +97,26 @@ def __get__( return wraps(self.func)(partial(self.func, obj)) @property - def name(self): + def name(self) -> str: """The name of the wrapped function.""" return self.func.__name__ @property - def path(self): + def path(self) -> str: """The path of the endpoint (relative to the Thing).""" return self._path or self.name @property - def title(self): + def title(self) -> str: """A human-readable title.""" return get_summary(self.func) or self.name @property - def description(self): + def description(self) -> str | None: """A description of the endpoint.""" return get_docstring(self.func, remove_summary=True) - def add_to_fastapi(self, app: FastAPI, thing: Thing): + def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: """Add an endpoint for this function to a FastAPI app. We will add an endpoint to the app, bound to a particular `.Thing`. @@ -126,11 +127,12 @@ def add_to_fastapi(self, app: FastAPI, thing: Thing): :param app: the `fastapi.FastAPI` application we are adding to. :param thing: the `.Thing` we're bound to. """ + assert thing.path is not None # fastapi_endpoint is equivalent to app.get/app.post/whatever fastapi_endpoint = getattr(app, self.http_method) bound_function = partial(self.func, thing) # NB the line above can't use self.__get__ as wraps() confuses FastAPI - kwargs = { # Auto-populate description and summary + kwargs: dict[str, Any] = { # Auto-populate description and summary "description": f"## {self.title}\n\n {self.description}", "summary": self.title, } From 7e870acabcad30a363551a3a0eb2e7ec3dca51f3 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 09/48] Add remaining type annotations. None of these should affect how the code actually runs. Return types of functions used as FastAPI endpoints are also specified as the `model` and thus I don't expect any change to the API. --- src/labthings_fastapi/example_things/__init__.py | 14 +++++++------- src/labthings_fastapi/outputs/blob.py | 3 ++- src/labthings_fastapi/outputs/mjpeg_stream.py | 10 +++++----- src/labthings_fastapi/server/__init__.py | 2 +- src/labthings_fastapi/server/cli.py | 2 +- src/labthings_fastapi/thing.py | 7 ++++--- .../thing_description/__init__.py | 2 +- src/labthings_fastapi/thing_description/_model.py | 2 +- src/labthings_fastapi/types/numpy.py | 2 +- typing_tests/thing_definitions.py | 4 ++-- 10 files changed, 25 insertions(+), 23 deletions(-) diff --git a/src/labthings_fastapi/example_things/__init__.py b/src/labthings_fastapi/example_things/__init__.py index def9bb5..9d6daba 100644 --- a/src/labthings_fastapi/example_things/__init__.py +++ b/src/labthings_fastapi/example_things/__init__.py @@ -74,7 +74,7 @@ def make_a_dict( return out @thing_action - def increment_counter(self): + def increment_counter(self) -> None: """Increment the counter property. This action doesn't do very much - all it does, in fact, @@ -84,7 +84,7 @@ def increment_counter(self): self.counter += 1 @thing_action - def slowly_increase_counter(self, increments: int = 60, delay: float = 1): + def slowly_increase_counter(self, increments: int = 60, delay: float = 1) -> None: """Increment the counter slowly over a minute. :param increments: how many times to increment. @@ -118,7 +118,7 @@ class ThingWithBrokenAffordances(Thing): """A Thing that raises exceptions in actions/properties.""" @thing_action - def broken_action(self): + def broken_action(self) -> None: """Do something that raises an exception. :raise RuntimeError: every time. @@ -126,7 +126,7 @@ def broken_action(self): raise RuntimeError("This is a broken action") @lt_property - def broken_property(self): + def broken_property(self) -> None: """Raise an exception when the property is accessed. :raise RuntimeError: every time. @@ -137,7 +137,7 @@ def broken_property(self): class ThingThatCantInstantiate(Thing): """A Thing that raises an exception in __init__.""" - def __init__(self): + def __init__(self) -> None: """Fail to initialise. :raise RuntimeError: every time. @@ -148,14 +148,14 @@ def __init__(self): class ThingThatCantStart(Thing): """A Thing that raises an exception in __enter__.""" - def __enter__(self): + def __enter__(self) -> None: """Fail to start the thing. :raise RuntimeError: every time. """ raise RuntimeError("This thing can't start") - def __exit__(self, exc_t: Any, exc_v: Any, exc_tb: Any): + def __exit__(self, exc_t: Any, exc_v: Any, exc_tb: Any) -> None: """Don't leave the thing as we never entered. :param exc_t: Exception type. diff --git a/src/labthings_fastapi/outputs/blob.py b/src/labthings_fastapi/outputs/blob.py index 420ad7f..201dfff 100644 --- a/src/labthings_fastapi/outputs/blob.py +++ b/src/labthings_fastapi/outputs/blob.py @@ -46,6 +46,7 @@ def get_image(self) -> MyImageBlob: import shutil from typing import ( Annotated, + Any, AsyncGenerator, Callable, Literal, @@ -215,7 +216,7 @@ class BlobFile: id: Optional[uuid.UUID] = None """A unique ID to identify the data in a `.BlobManager`.""" - def __init__(self, file_path: str, media_type: str, **kwargs): + def __init__(self, file_path: str, media_type: str, **kwargs: Any): r"""Create a `.BlobFile` to wrap data stored on disk. `.BlobFile` objects wrap data stored on disk as files. They diff --git a/src/labthings_fastapi/outputs/mjpeg_stream.py b/src/labthings_fastapi/outputs/mjpeg_stream.py index 3b0d330..efdd882 100644 --- a/src/labthings_fastapi/outputs/mjpeg_stream.py +++ b/src/labthings_fastapi/outputs/mjpeg_stream.py @@ -139,7 +139,7 @@ def __init__(self, ringbuffer_size: int = 10): self._ringbuffer: list[RingbufferEntry] = [] self.reset(ringbuffer_size=ringbuffer_size) - def reset(self, ringbuffer_size: Optional[int] = None): + def reset(self, ringbuffer_size: Optional[int] = None) -> None: """Reset the stream and optionally change the ringbuffer size. Discard all frames from the ringbuffer and reset the frame index. @@ -361,7 +361,7 @@ class MJPEGStreamDescriptor: This descriptor does not currently show up in the :ref:`wot_td`. """ - def __init__(self, **kwargs: dict[str, Any]): + def __init__(self, **kwargs: Any): r"""Initialise an MJPEGStreamDescriptor. :param \**kwargs: keyword arguments are passed to the initialiser of @@ -381,10 +381,10 @@ def __set_name__(self, _owner: Thing, name: str) -> None: self.name = name @overload - def __get__(self, obj: Literal[None], type=None) -> Self: ... # noqa: D105 + def __get__(self, obj: Literal[None], type: type | None = None) -> Self: ... # noqa: D105 @overload - def __get__(self, obj: Thing, type=None) -> MJPEGStream: ... # noqa: D105 + def __get__(self, obj: Thing, type: type | None = None) -> MJPEGStream: ... # noqa: D105 def __get__( self, obj: Optional[Thing], type: type[Thing] | None = None @@ -452,5 +452,5 @@ class Camera(lt.Thing): f"{thing.path}{self.name}/viewer", response_class=HTMLResponse, ) - async def viewer_page(): + async def viewer_page() -> HTMLResponse: return await self.viewer_page(f"{thing.path}{self.name}") diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 0ef9abd..12763d7 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -216,7 +216,7 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None]: self.blocking_portal = None - def add_things_view_to_app(self): + def add_things_view_to_app(self) -> None: """Add an endpoint that shows the list of attached things.""" thing_server = self diff --git a/src/labthings_fastapi/server/cli.py b/src/labthings_fastapi/server/cli.py index 5a1aefb..d8e32ba 100644 --- a/src/labthings_fastapi/server/cli.py +++ b/src/labthings_fastapi/server/cli.py @@ -30,7 +30,7 @@ from . import ThingServer, server_from_config -def get_default_parser(): +def get_default_parser() -> ArgumentParser: """Return the default CLI parser for LabThings. This can be used to add more arguments, for custom CLIs that make use of diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 3d3cb73..c2ca49d 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -152,7 +152,7 @@ def thing_description(request: Request) -> ThingDescription: return self.thing_description(base=str(request.base_url)) @server.app.websocket(self.path + "ws") - async def websocket(ws: WebSocket): + async def websocket(ws: WebSocket) -> None: await websocket_endpoint(self, ws) # A private variable to hold the list of settings so it doesn't need to be @@ -230,13 +230,14 @@ def load_settings(self, setting_storage_path: str) -> None: _LOGGER.warning("Error loading settings for %s", thing_name) self._setting_storage_path = setting_storage_path - def save_settings(self): + def save_settings(self) -> None: """Save settings to JSON. This is called whenever a setting is updated. All settings are written to the settings file every time. """ if self._settings is not None: + assert self._setting_storage_path is not None setting_dict = {} for name in self._settings.keys(): value = getattr(self, name) @@ -352,7 +353,7 @@ def observe_property(self, property_name: str, stream: ObjectSendStream) -> None raise KeyError(f"{property_name} is not a LabThings Property") prop._observers_set(self).add(stream) - def observe_action(self, action_name: str, stream: ObjectSendStream): + def observe_action(self, action_name: str, stream: ObjectSendStream) -> None: """Register a stream to receive action status change notifications. :param action_name: the action to register for. diff --git a/src/labthings_fastapi/thing_description/__init__.py b/src/labthings_fastapi/thing_description/__init__.py index a06497f..5691c83 100644 --- a/src/labthings_fastapi/thing_description/__init__.py +++ b/src/labthings_fastapi/thing_description/__init__.py @@ -278,7 +278,7 @@ def jsonschema_to_dataschema( return output -def type_to_dataschema(t: type, **kwargs) -> DataSchema: +def type_to_dataschema(t: type, **kwargs: Any) -> DataSchema: r"""Convert a Python type to a Thing Description DataSchema. This makes use of pydantic's `schema_of` function to create a diff --git a/src/labthings_fastapi/thing_description/_model.py b/src/labthings_fastapi/thing_description/_model.py index f5bdcf9..8c3c163 100644 --- a/src/labthings_fastapi/thing_description/_model.py +++ b/src/labthings_fastapi/thing_description/_model.py @@ -81,7 +81,7 @@ class Subprotocol(Enum): ] -def uses_thing_context(v: ThingContextType): +def uses_thing_context(v: ThingContextType) -> None: """Check the URLs in the ThingContextType are valid. This function makes ``assert`` statements, so will fail with an exception diff --git a/src/labthings_fastapi/types/numpy.py b/src/labthings_fastapi/types/numpy.py index e8d067f..2fb27db 100644 --- a/src/labthings_fastapi/types/numpy.py +++ b/src/labthings_fastapi/types/numpy.py @@ -71,7 +71,7 @@ def np_to_listoflists(arr: np.ndarray) -> NestedListOfNumbers: :param arr: a `numpy.ndarray`. :return: a nested list of numbers. """ - return arr.tolist() # type: ignore[return-value] + return arr.tolist() def listoflists_to_np(lol: Union[NestedListOfNumbers, np.ndarray]) -> np.ndarray: diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index b0815e9..5650f2f 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -59,7 +59,7 @@ def int_factory() -> int: @lt.property -def strprop(self) -> str: +def strprop(self: typing.Any) -> str: """A functional property that should not cause mypy errors.""" return "foo" @@ -213,7 +213,7 @@ def intprop2(self) -> int: return 0 @intprop2.setter - def set_intprop2(self, value: int): + def set_intprop2(self, value: int) -> None: """Setter for intprop2.""" pass From 60cc4848b1d72d8b83301201a9895a33e75f9cb2 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 14 Aug 2025 23:56:23 +0100 Subject: [PATCH 10/48] Use paths configured in pyproject.toml in CI This also combines the two runs of mypy on src and typing_tests, as they can now both use the stricter rules. --- .github/workflows/test.yml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 21d449c..6a174a0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -74,10 +74,7 @@ jobs: path: ./coverage.lcov - name: Analyse with MyPy - run: mypy src - - - name: Type tests with MyPy - run: mypy --warn-unused-ignores typing_tests + run: mypy test-with-unpinned-deps: runs-on: ubuntu-latest @@ -105,7 +102,7 @@ jobs: - name: Analyse with MyPy if: success() || failure() - run: mypy src + run: mypy - name: Test with pytest if: success() || failure() From d3577052978a2d1049ce5f27729c2f81eacea820 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 15 Aug 2025 00:11:50 +0100 Subject: [PATCH 11/48] Use `.value` instead of `str` to convert enum to string. Using `str` was converting `pending` to `InvocationStatus.PENDING` which caused the websocket tests to fail. Using `.value` fixes the problem. --- src/labthings_fastapi/actions/__init__.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/labthings_fastapi/actions/__init__.py b/src/labthings_fastapi/actions/__init__.py index e278fab..b526ea2 100644 --- a/src/labthings_fastapi/actions/__init__.py +++ b/src/labthings_fastapi/actions/__init__.py @@ -255,7 +255,7 @@ def run(self) -> None: # which thinks we are calling ActionDescriptor.__get__. action: ActionDescriptor = self.action # type: ignore[call-overload] try: - action.emit_changed_event(self.thing, str(self._status)) + action.emit_changed_event(self.thing, self._status.value) # Capture just this thread's log messages handler = DequeLogHandler(dest=self._log) @@ -269,7 +269,7 @@ def run(self) -> None: with self._status_lock: self._status = InvocationStatus.RUNNING self._start_time = datetime.datetime.now() - action.emit_changed_event(self.thing, str(self._status)) + action.emit_changed_event(self.thing, self._status.value) # The next line actually runs the action. ret = action.__get__(thing)(**kwargs, **self.dependencies) @@ -277,12 +277,12 @@ def run(self) -> None: with self._status_lock: self._return_value = ret self._status = InvocationStatus.COMPLETED - action.emit_changed_event(self.thing, str(self._status)) + action.emit_changed_event(self.thing, self._status.value) except InvocationCancelledError: logger.info(f"Invocation {self.id} was cancelled.") with self._status_lock: self._status = InvocationStatus.CANCELLED - action.emit_changed_event(self.thing, str(self._status)) + action.emit_changed_event(self.thing, self._status.value) except Exception as e: # skipcq: PYL-W0703 # First log if isinstance(e, InvocationError): @@ -294,7 +294,7 @@ def run(self) -> None: with self._status_lock: self._status = InvocationStatus.ERROR self._exception = e - action.emit_changed_event(self.thing, str(self._status)) + action.emit_changed_event(self.thing, self._status.value) finally: with self._status_lock: self._end_time = datetime.datetime.now() From 6e8e2797e253ae16a6faf8dc3fd0fd6dc154cf8d Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 15 Aug 2025 00:29:43 +0100 Subject: [PATCH 12/48] Remove unnecessary type:ignore comments These weren't flagged by dmypy but did show up with mypy. --- src/labthings_fastapi/utilities/introspection.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/labthings_fastapi/utilities/introspection.py b/src/labthings_fastapi/utilities/introspection.py index 8cbeaf6..f991f33 100644 --- a/src/labthings_fastapi/utilities/introspection.py +++ b/src/labthings_fastapi/utilities/introspection.py @@ -114,9 +114,7 @@ def input_model_from_signature( p_type = Any if p.annotation is Parameter.empty else type_hints[name] # pydantic uses `...` to represent missing defaults (i.e. required params) default = Field(...) if p.default is Parameter.empty else p.default - # p_type below has a complicated type, but it is reasonable to - # call p_type a `type` and ignore the mypy error. - fields[name] = (p_type, default) # type: ignore[assignment] + fields[name] = (p_type, default) model = create_model( # type: ignore[call-overload] f"{func.__name__}_input", model_config=ConfigDict(extra="allow" if takes_v_kwargs else "forbid"), @@ -176,7 +174,7 @@ def return_type(func: Callable) -> Type: """ sig = inspect.signature(func) if sig.return_annotation == inspect.Signature.empty: - return Any # type: ignore[return-value] + return Any else: # We use `get_type_hints` rather than just `sig.return_annotation` # because it resolves forward references, etc. From 46ff8e67372611fd3f81a01e494d39b721e453d8 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 15 Aug 2025 02:48:06 +0100 Subject: [PATCH 13/48] Test the stub BaseProperty.__set__ I added a __set__ to BaseProperty to satisfy mypy, so now there is a test to check it raises an error and is overridden. --- tests/test_property.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_property.py b/tests/test_property.py index aad5db5..2ffac7f 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -220,6 +220,20 @@ class Example: assert set(entry.keys()) == set(["get", "put"]) +def test_baseproperty_set_error(): + """Check `.Baseproperty.__set__` raises an error and is overridden.""" + assert tp.DataProperty.__get__ is tp.BaseProperty.__get__ + assert tp.DataProperty.__set__ is not tp.BaseProperty.__set__ + assert tp.FunctionalProperty.__set__ is not tp.BaseProperty.__set__ + + class Example: + bp: int = tp.BaseProperty() + + example = Example() + with pytest.raises(NotImplementedError): + example.bp = 0 + + def test_decorator_exception(): r"""Check decorators work as expected when the setter has a different name. From f6c89764fec2f2ac856ff832e0e752205a323a2c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Sun, 24 Aug 2025 20:45:24 +0200 Subject: [PATCH 14/48] Add a comment explaining ClientBlobOutput in action inputs. --- src/labthings_fastapi/client/__init__.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/labthings_fastapi/client/__init__.py b/src/labthings_fastapi/client/__init__.py index 0aeaee5..9c5ce62 100644 --- a/src/labthings_fastapi/client/__init__.py +++ b/src/labthings_fastapi/client/__init__.py @@ -182,6 +182,14 @@ def invoke_action(self, path: str, **kwargs: Any) -> Any: for k in kwargs.keys(): value = kwargs[k] if isinstance(value, ClientBlobOutput): + # ClientBlobOutput objects may be used as input to a subsequent + # action. When this is done, they should be serialised to a dict + # with `href` and `media_type` keys, as done below. + # Ideally this should be replaced with `Blob` and the use of + # `pydantic` models to serialise action inputs. + # + # Note that the blob will not be uploaded: we rely on the blob + # still existing on the server. kwargs[k] = {"href": value.href, "media_type": value.media_type} r = self.client.post(urljoin(self.path, path), json=kwargs) r.raise_for_status() From c539720658d01c57b1279de26ecbde8dab2978e2 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Sun, 24 Aug 2025 20:50:51 +0200 Subject: [PATCH 15/48] Remove an assertion --- src/labthings_fastapi/descriptors/endpoint.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/labthings_fastapi/descriptors/endpoint.py b/src/labthings_fastapi/descriptors/endpoint.py index 47c542c..cf9aa7e 100644 --- a/src/labthings_fastapi/descriptors/endpoint.py +++ b/src/labthings_fastapi/descriptors/endpoint.py @@ -16,6 +16,8 @@ from __future__ import annotations from functools import partial, wraps +from labthings_fastapi.exceptions import NotConnectedToServerError + from ..utilities.introspection import get_docstring, get_summary from typing import ( @@ -127,7 +129,12 @@ def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: :param app: the `fastapi.FastAPI` application we are adding to. :param thing: the `.Thing` we're bound to. """ - assert thing.path is not None + if thing.path is None: + raise NotConnectedToServerError( + "Endpoints may only be added to Things that have a path. " + "This error indicates `add_to_fastapi` is being called before " + "the Thing has been assigned a path, which should not happen." + ) # fastapi_endpoint is equivalent to app.get/app.post/whatever fastapi_endpoint = getattr(app, self.http_method) bound_function = partial(self.func, thing) From 1490501da49fa797088bd462c67fd758176dcc99 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 13:56:28 +0200 Subject: [PATCH 16/48] Fix endpoint decorator docstring. Added the NotConnectedToServerError exception. --- src/labthings_fastapi/descriptors/endpoint.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/labthings_fastapi/descriptors/endpoint.py b/src/labthings_fastapi/descriptors/endpoint.py index cf9aa7e..b738ea4 100644 --- a/src/labthings_fastapi/descriptors/endpoint.py +++ b/src/labthings_fastapi/descriptors/endpoint.py @@ -128,12 +128,16 @@ def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: :param app: the `fastapi.FastAPI` application we are adding to. :param thing: the `.Thing` we're bound to. + + :raises NotConnectedToServerError: if there is no ``path`` attribute + of the host `.Thing` (which usually means it is not yet connected + to a server). """ if thing.path is None: raise NotConnectedToServerError( - "Endpoints may only be added to Things that have a path. " - "This error indicates `add_to_fastapi` is being called before " - "the Thing has been assigned a path, which should not happen." + "Attempted to add an endpoint to the API, but there is no " + "path set on the Thing. This usually means it is not connected " + "to a ThingServer." ) # fastapi_endpoint is equivalent to app.get/app.post/whatever fastapi_endpoint = getattr(app, self.http_method) From 12b76dd53f449e63196e32db8e5011da63d6dea7 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:26:27 +0200 Subject: [PATCH 17/48] Improve test coverage of endpoint decorator I test for an error condition, and also test using the endpoint functions not via HTTP. It was necessary to add a default value for Thing.path to make sure we raise the right error - this is needed by other checks that are introduced in #180. --- src/labthings_fastapi/thing.py | 2 +- tests/test_endpoint_decorator.py | 39 ++++++++++++++++++++++++++------ 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index c2ca49d..3be3631 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -77,7 +77,7 @@ class Thing: """A human-readable description of the Thing""" _labthings_blocking_portal: Optional[BlockingPortal] = None """See :ref:`concurrency` for why blocking portal is needed.""" - path: Optional[str] + path: Optional[str] = None """The path at which the `.Thing` is exposed over HTTP.""" async def __aenter__(self) -> Self: diff --git a/tests/test_endpoint_decorator.py b/tests/test_endpoint_decorator.py index 0e96322..6d4784b 100644 --- a/tests/test_endpoint_decorator.py +++ b/tests/test_endpoint_decorator.py @@ -1,6 +1,7 @@ from fastapi.testclient import TestClient -from labthings_fastapi import ThingServer, Thing, fastapi_endpoint from pydantic import BaseModel +import pytest +import labthings_fastapi as lt class PostBodyModel(BaseModel): @@ -8,32 +9,56 @@ class PostBodyModel(BaseModel): b: int -class TestThing(Thing): - @fastapi_endpoint("get") +class MyThing(lt.Thing): + @lt.fastapi_endpoint("get") def path_from_name(self) -> str: return "path_from_name" - @fastapi_endpoint("get", path="path_from_path") + @lt.fastapi_endpoint("get", path="path_from_path") def get_method(self) -> str: return "get_method" - @fastapi_endpoint("post", path="path_from_path") + @lt.fastapi_endpoint("post", path="path_from_path") def post_method(self, body: PostBodyModel) -> str: return f"post_method {body.a} {body.b}" def test_endpoints(): - server = ThingServer() - server.add_thing(TestThing(), "/thing") + """Check endpoints may be added to the app and work as expected.""" + server = lt.ThingServer() + thing = MyThing() + server.add_thing(thing, "/thing") with TestClient(server.app) as client: + # Check the function works when used directly + assert thing.path_from_name() == "path_from_name" + # Check it works identically over HTTP. The path is + # generated from the name of the function. r = client.get("/thing/path_from_name") r.raise_for_status() assert r.json() == "path_from_name" + # get_method has an explicit path - check it can be + # used both directly and via that path. + assert thing.get_method() == "get_method" r = client.get("/thing/path_from_path") r.raise_for_status() assert r.json() == "get_method" + # post_method uses the same path, for a different + # function + assert thing.post_method(PostBodyModel(a=1, b=2)) == "post_method 1 2" r = client.post("/thing/path_from_path", json={"a": 1, "b": 2}) r.raise_for_status() assert r.json() == "post_method 1 2" + + +def test_endpoint_notconnected(mocker): + """Check for the correct error if we add endpoints prematurely. + + We should get this error if we call ``add_to_fastapi`` on an endpoint + where the `.Thing` does not have a valid ``path`` attribute. + """ + thing = MyThing() + + with pytest.raises(lt.exceptions.NotConnectedToServerError): + MyThing.get_method.add_to_fastapi(mocker.Mock(), thing) From 198e3be78fbe08f5ce85f912a7a97291fe8018c6 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:16 +0200 Subject: [PATCH 18/48] Increase ruff strictness --- pyproject.toml | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 7b002e3..4eb7a8b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -77,7 +77,16 @@ docstring-code-format = true [tool.ruff.lint] external = ["DOC401", "F824", "DOC101", "DOC103"] # used via flake8/pydoclint -select = ["B", "E4", "E7", "E9", "F", "D", "DOC"] +select = [ + "ANN", # type annotations (ensuring they are present, complements mypy) + "B", # flake8-bugbear + "E", # flake8 + "F", # flake8 + "FAST", # FastAPI rules (mostly around dependencies) + "D", # pydoclint (recommended by the developers over flake8 plugin) + "DOC", # pydocstyle (limited support for sphinx style: see ignores) + "FAST" # FastAPI rules +] ignore = [ "D203", # incompatible with D204 "D213", # incompatible with D212 @@ -88,12 +97,14 @@ ignore = [ "B008", # This disallows function calls in default values. # FastAPI Depends() breaks this rule, and FastAPI's response is "disable it". # see https://github.com/fastapi/fastapi/issues/1522 + "ANN401", # ANN401 disallows Any. There are quite a few places where Any is + # needed for dynamically typed code. ] preview = true [tool.ruff.lint.per-file-ignores] # Tests are currently not fully docstring-ed, we'll ignore this for now. -"tests/*" = ["D", "DOC"] +"tests/*" = ["D", "DOC", "ANN"] # Typing tests do have docstrings, but it's not helpful to insist on imperative # mood etc. "typing_tests/*" = ["D404", "D401"] From 3ac154255bcb936bac5d31bc464b1652fac31900 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:17 +0200 Subject: [PATCH 19/48] Use only Annotated form of FastAPI Depends() FastAPI allows dependencies to be specified either with an annotated type, or by supplying Depends() as the default. The latter form can cause problems, e.g. by masking the fact that a dependency hasn't been injected. I've now changed all occurrences of `Depends()` to use annotated types instead, as required by the `FAST` ruleset in ruff. --- tests/test_dependencies.py | 2 +- tests/test_dependencies_2.py | 6 +++--- typing_tests/thing_definitions.py | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/test_dependencies.py b/tests/test_dependencies.py index f33d634..d30c63e 100644 --- a/tests/test_dependencies.py +++ b/tests/test_dependencies.py @@ -53,7 +53,7 @@ class DepClass: sub: Request @app.post("/dep") - def endpoint(id: DepClass = Depends()) -> bool: + def endpoint(id: Annotated[DepClass, Depends()]) -> bool: return True with TestClient(app) as client: diff --git a/tests/test_dependencies_2.py b/tests/test_dependencies_2.py index 5f682d6..1bdc71d 100644 --- a/tests/test_dependencies_2.py +++ b/tests/test_dependencies_2.py @@ -75,7 +75,7 @@ def test_fancy_id_default(): app = FastAPI() @app.post("/invoke_fancy") - def invoke_fancy(id: FancyID = Depends()) -> dict: + def invoke_fancy(id: Annotated[FancyID, Depends()]) -> dict: return {"id": "me"} with TestClient(app) as client: @@ -93,7 +93,7 @@ class DepClass: pass @app.post("/dep") - def endpoint(id: DepClass = Depends()) -> bool: + def endpoint(id: Annotated[DepClass, Depends()]) -> bool: return True with TestClient(app) as client: @@ -123,7 +123,7 @@ def __init__(self, sub: Annotated[SubDepClass, Depends()]): self.sub = sub @app.post("/dep") - def endpoint(id: DepClass = Depends()) -> bool: + def endpoint(id: Annotated[DepClass, Depends()]) -> bool: assert isinstance(id.sub, SubDepClass) return True diff --git a/typing_tests/thing_definitions.py b/typing_tests/thing_definitions.py index 5650f2f..553e486 100644 --- a/typing_tests/thing_definitions.py +++ b/typing_tests/thing_definitions.py @@ -139,7 +139,7 @@ class TestExplicitDescriptor(lt.Thing): """A DataProperty that should not cause mypy errors.""" intprop2 = lt.DataProperty[int](default_factory=int_factory) - """This property should not cause mypy errors, as the factory matches the type hint.""" + """The factory matches the type hint, so this should be OK.""" intprop3 = lt.DataProperty[int](default_factory=optional_int_factory) """Uses a factory function that doesn't match the type hint. @@ -151,7 +151,7 @@ class TestExplicitDescriptor(lt.Thing): """ intprop4 = lt.DataProperty[int](default="foo") # type: ignore[call-overload] - """This property should cause mypy to throw an error, as the default is a string.""" + """This property should cause an error, as the default is a string.""" intprop5 = lt.DataProperty[int]() # type: ignore[call-overload] """This property should cause mypy to throw an error, as it has no default.""" @@ -160,7 +160,7 @@ class TestExplicitDescriptor(lt.Thing): """A DataProperty that should not cause mypy errors.""" optionalintprop2 = lt.DataProperty[int | None](default_factory=optional_int_factory) - """This property should not cause mypy errors, as the factory matches the type hint.""" + """This property should not cause mypy errors: the factory matches the type hint.""" optionalintprop3 = lt.DataProperty[int | None](default_factory=int_factory) """Uses a factory function that is a subset of the type hint.""" From 11faed24f944c01ae3fcb3fca1921e49846406dc Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:17 +0200 Subject: [PATCH 20/48] Exempt sphinx config from annotations rules. Sphinx is configured with a Python script, but this is not part of the package, the examples, or the test code. I've therefore excluded it from the rules. I've also changed the exclusion to be specific to the docs config file, and not the whole docs folder, just in case we add scripts there in the future. This also means any example Python files will be checked. --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 4eb7a8b..8934b8b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -108,7 +108,7 @@ preview = true # Typing tests do have docstrings, but it's not helpful to insist on imperative # mood etc. "typing_tests/*" = ["D404", "D401"] -"docs/*" = ["D", "DOC"] +"docs/source/conf.py" = ["D", "DOC", "ANN"] [tool.ruff.lint.pydocstyle] # This lets the D401 checker understand that decorated thing properties and thing From b1b5644d7e9f1711cf8e1123855471b9d30b615c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:17 +0200 Subject: [PATCH 21/48] Fix docstrings in example code I've added module docstrings and some full stops, so the docstrings in the example comply with the rules for the module source. --- docs/source/quickstart/counter.py | 10 ++++++---- docs/source/quickstart/counter_client.py | 2 ++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/source/quickstart/counter.py b/docs/source/quickstart/counter.py index 10f8a82..8a2c84f 100644 --- a/docs/source/quickstart/counter.py +++ b/docs/source/quickstart/counter.py @@ -1,13 +1,15 @@ +"""An example Thing that implements a counter.""" + import time import labthings_fastapi as lt class TestThing(lt.Thing): - """A test thing with a counter property and a couple of actions""" + """A test thing with a counter property and a couple of actions.""" @lt.thing_action def increment_counter(self) -> None: - """Increment the counter property + """Increment the counter property. This action doesn't do very much - all it does, in fact, is increment the counter (which may be read using the @@ -17,13 +19,13 @@ def increment_counter(self) -> None: @lt.thing_action def slowly_increase_counter(self) -> None: - """Increment the counter slowly over a minute""" + """Increment the counter slowly over a minute.""" for _i in range(60): time.sleep(1) self.increment_counter() counter: int = lt.property(default=0, readonly=True) - "A pointless counter" + "A pointless counter." if __name__ == "__main__": diff --git a/docs/source/quickstart/counter_client.py b/docs/source/quickstart/counter_client.py index bb24ab1..b39941b 100644 --- a/docs/source/quickstart/counter_client.py +++ b/docs/source/quickstart/counter_client.py @@ -1,3 +1,5 @@ +"""Client code that interacts with the counter Thing over HTTP.""" + from labthings_fastapi import ThingClient counter = ThingClient.from_url("http://localhost:5000/counter/") From 12e7940d8f922dbff5dba40aa137531664af6a4a Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:17 +0200 Subject: [PATCH 22/48] Add return annotations for __init__ Return type annotations for __init__ are not overly useful, and are not required by `mypy`. However, other magic methods do need an annotation, and it's helpful to have `ruff` check this rather than waiting for `mypy`. I've annotated all remaining `__init__` methods so this linter rule now passes. I also fixed a missing import from the change to Annotated for FastAPI dependencies. --- src/labthings_fastapi/actions/__init__.py | 4 ++-- src/labthings_fastapi/client/__init__.py | 2 +- src/labthings_fastapi/client/in_server.py | 4 ++-- src/labthings_fastapi/client/outputs.py | 2 +- src/labthings_fastapi/dependencies/invocation.py | 2 +- src/labthings_fastapi/descriptors/action.py | 2 +- src/labthings_fastapi/descriptors/endpoint.py | 2 +- src/labthings_fastapi/outputs/blob.py | 4 ++-- src/labthings_fastapi/outputs/mjpeg_stream.py | 8 +++++--- src/labthings_fastapi/properties.py | 2 +- src/labthings_fastapi/server/__init__.py | 2 +- tests/test_dependencies.py | 1 + 12 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/labthings_fastapi/actions/__init__.py b/src/labthings_fastapi/actions/__init__.py index b526ea2..8e37930 100644 --- a/src/labthings_fastapi/actions/__init__.py +++ b/src/labthings_fastapi/actions/__init__.py @@ -78,7 +78,7 @@ def __init__( dependencies: Optional[dict[str, Any]] = None, log_len: int = 1000, cancel_hook: Optional[CancelHook] = None, - ): + ) -> None: """Create a thread to run an action and track its outputs. :param action: provides the function that we run, as well as metadata @@ -312,7 +312,7 @@ def __init__( self, dest: MutableSequence, level: int = logging.INFO, - ): + ) -> None: """Set up a log handler that appends messages to a deque. .. warning:: diff --git a/src/labthings_fastapi/client/__init__.py b/src/labthings_fastapi/client/__init__.py index 9c5ce62..07fdb74 100644 --- a/src/labthings_fastapi/client/__init__.py +++ b/src/labthings_fastapi/client/__init__.py @@ -121,7 +121,7 @@ class ThingClient: creates a subclass with the right attributes. """ - def __init__(self, base_url: str, client: Optional[httpx.Client] = None): + def __init__(self, base_url: str, client: Optional[httpx.Client] = None) -> None: """Create a ThingClient connected to a remote Thing. :param base_url: the base URL of the Thing. This should be the URL diff --git a/src/labthings_fastapi/client/in_server.py b/src/labthings_fastapi/client/in_server.py index 9c3d6ff..d81de10 100644 --- a/src/labthings_fastapi/client/in_server.py +++ b/src/labthings_fastapi/client/in_server.py @@ -54,7 +54,7 @@ class DirectThingClient: thing_path: str """The path to the Thing on the server. Relative to the server's base URL.""" - def __init__(self, request: Request, **dependencies: Mapping[str, Any]): + def __init__(self, request: Request, **dependencies: Mapping[str, Any]) -> None: r"""Wrap a `.Thing` so it works like a `.ThingClient`. This class is designed to be used as a FastAPI dependency, and will @@ -157,7 +157,7 @@ class DependencyNameClashError(KeyError): exception is raised. """ - def __init__(self, name: str, existing: type, new: type): + def __init__(self, name: str, existing: type, new: type) -> None: """Create a DependencyNameClashError. See class docstring for an explanation of the error. diff --git a/src/labthings_fastapi/client/outputs.py b/src/labthings_fastapi/client/outputs.py index 061e9ca..09c4962 100644 --- a/src/labthings_fastapi/client/outputs.py +++ b/src/labthings_fastapi/client/outputs.py @@ -35,7 +35,7 @@ class ClientBlobOutput: def __init__( self, media_type: str, href: str, client: Optional[httpx.Client] = None - ): + ) -> None: """Create a ClientBlobOutput to wrap a link to a downloadable file. :param media_type: the MIME type of the remote file. diff --git a/src/labthings_fastapi/dependencies/invocation.py b/src/labthings_fastapi/dependencies/invocation.py index 85b0bf6..8ed80f7 100644 --- a/src/labthings_fastapi/dependencies/invocation.py +++ b/src/labthings_fastapi/dependencies/invocation.py @@ -137,7 +137,7 @@ class CancelEvent(threading.Event): usually by a ``DELETE`` request to the invocation's URL. """ - def __init__(self, id: InvocationID): + def __init__(self, id: InvocationID) -> None: """Initialise the cancellation event. :param id: The invocation ID, annotated as a dependency so it is diff --git a/src/labthings_fastapi/descriptors/action.py b/src/labthings_fastapi/descriptors/action.py index 7c06d89..a61dd82 100644 --- a/src/labthings_fastapi/descriptors/action.py +++ b/src/labthings_fastapi/descriptors/action.py @@ -84,7 +84,7 @@ def __init__( func: Callable, response_timeout: float = 1, retention_time: float = 300, - ): + ) -> None: """Create a new action descriptor. The action descriptor wraps a method of a `.Thing`. It may still be diff --git a/src/labthings_fastapi/descriptors/endpoint.py b/src/labthings_fastapi/descriptors/endpoint.py index b738ea4..7545569 100644 --- a/src/labthings_fastapi/descriptors/endpoint.py +++ b/src/labthings_fastapi/descriptors/endpoint.py @@ -49,7 +49,7 @@ def __init__( http_method: HTTPMethod = "get", path: Optional[str] = None, **kwargs: Mapping[str, Any], - ): + ) -> None: r"""Initialise an EndpointDescriptor. See `.fastapi_endpoint`, which is the usual way of instantiating this diff --git a/src/labthings_fastapi/outputs/blob.py b/src/labthings_fastapi/outputs/blob.py index 201dfff..cd95b16 100644 --- a/src/labthings_fastapi/outputs/blob.py +++ b/src/labthings_fastapi/outputs/blob.py @@ -154,7 +154,7 @@ class BlobBytes: id: Optional[uuid.UUID] = None """A unique ID to identify the data in a `.BlobManager`.""" - def __init__(self, data: bytes, media_type: str): + def __init__(self, data: bytes, media_type: str) -> None: """Create a `.BlobBytes` object. `.BlobBytes` objects wrap data stored in memory as `bytes`. They @@ -216,7 +216,7 @@ class BlobFile: id: Optional[uuid.UUID] = None """A unique ID to identify the data in a `.BlobManager`.""" - def __init__(self, file_path: str, media_type: str, **kwargs: Any): + def __init__(self, file_path: str, media_type: str, **kwargs: Any) -> None: r"""Create a `.BlobFile` to wrap data stored on disk. `.BlobFile` objects wrap data stored on disk as files. They diff --git a/src/labthings_fastapi/outputs/mjpeg_stream.py b/src/labthings_fastapi/outputs/mjpeg_stream.py index efdd882..c500dce 100644 --- a/src/labthings_fastapi/outputs/mjpeg_stream.py +++ b/src/labthings_fastapi/outputs/mjpeg_stream.py @@ -61,7 +61,9 @@ class MJPEGStreamResponse(StreamingResponse): media_type = "multipart/x-mixed-replace; boundary=frame" """The media_type used to describe the endpoint in FastAPI.""" - def __init__(self, gen: AsyncGenerator[bytes, None], status_code: int = 200): + def __init__( + self, gen: AsyncGenerator[bytes, None], status_code: int = 200 + ) -> None: """Set up StreamingResponse that streams an MJPEG stream. This response is initialised with an async generator that yields `bytes` @@ -124,7 +126,7 @@ class MJPEGStream: of new frames, and then retrieving the frame (shortly) afterwards. """ - def __init__(self, ringbuffer_size: int = 10): + def __init__(self, ringbuffer_size: int = 10) -> None: """Initialise an MJPEG stream. See the class docstring for `.MJPEGStream`. Note that it will @@ -361,7 +363,7 @@ class MJPEGStreamDescriptor: This descriptor does not currently show up in the :ref:`wot_td`. """ - def __init__(self, **kwargs: Any): + def __init__(self, **kwargs: Any) -> None: r"""Initialise an MJPEGStreamDescriptor. :param \**kwargs: keyword arguments are passed to the initialiser of diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index a88cbe4..84b6ca0 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -687,7 +687,7 @@ class FunctionalProperty(BaseProperty[Value], Generic[Value]): def __init__( self, fget: ValueGetter, - ): + ) -> None: """Set up a FunctionalProperty. Create a descriptor for a property that uses a getter function. diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 12763d7..b4e015d 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -53,7 +53,7 @@ class ThingServer: an `anyio.from_thread.BlockingPortal`. """ - def __init__(self, settings_folder: Optional[str] = None): + def __init__(self, settings_folder: Optional[str] = None) -> None: """Initialise a LabThings server. Setting up the `.ThingServer` involves creating the underlying diff --git a/tests/test_dependencies.py b/tests/test_dependencies.py index d30c63e..ed800bf 100644 --- a/tests/test_dependencies.py +++ b/tests/test_dependencies.py @@ -5,6 +5,7 @@ """ from dataclasses import dataclass +from typing import Annotated from fastapi import Depends, FastAPI, Request from labthings_fastapi.deps import InvocationID from fastapi.testclient import TestClient From b6069f4906fad1b6d14419f8796f732807b5433b Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:17 +0200 Subject: [PATCH 23/48] Eliminate assert statements from base_descriptor One assertion has simply been deleted (line 437) as it was not clear how it could ever fail. An explicit type check a couple of lines below ensures we will catch any errors here promptly, so robustness is not affected. --- src/labthings_fastapi/base_descriptor.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 919a1f0..0a370cd 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -244,11 +244,16 @@ def name(self) -> str: The ``name`` of :ref:`wot_affordances` is used in their URL and in the :ref:`gen_docs` served by LabThings. + + :raises DescriptorNotAddedToClassError: if ``__set_name__`` has not yet + been called. """ self.assert_set_name_called() - assert self._name is not None - # The assert statement is mostly for typing: if assert_set_name_called - # doesn't raise an error, self._name has been set. + if self._name is None: # pragma: no cover + raise DescriptorNotAddedToClassError("`_name` is not set.") + # The exception is mostly for typing: if `assert_set_name_called`` + # doesn't raise an error, `BaseDescriptor.__set_name__` has been + # called and thus `self._name`` has been set. return self._name @property @@ -429,7 +434,6 @@ class Example: return {} # The line below parses the class to get a syntax tree. module_ast = ast.parse(textwrap.dedent(src)) - assert isinstance(module_ast, ast.Module) class_def = module_ast.body[0] if not isinstance(class_def, ast.ClassDef): raise TypeError("The object supplied was not a class.") From 72b3cf612ce875842b4e0595b0be547756654a5d Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:17 +0200 Subject: [PATCH 24/48] Replaced assertions in actions/__init__ with exceptions I've exempted these from coverage: they are both primarily for the benefit of mypy, as they only guard against weakrefs failing. The objects referenced won't be deleted until after the server shuts down, so these errors should never occur. --- src/labthings_fastapi/actions/__init__.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/labthings_fastapi/actions/__init__.py b/src/labthings_fastapi/actions/__init__.py index 8e37930..20b47dc 100644 --- a/src/labthings_fastapi/actions/__init__.py +++ b/src/labthings_fastapi/actions/__init__.py @@ -169,9 +169,17 @@ def status(self) -> InvocationStatus: @property def action(self) -> ActionDescriptor: - """The `.ActionDescriptor` object running in this thread.""" + """The `.ActionDescriptor` object running in this thread. + + :raises RuntimeError: if the action descriptor has been deleted. + This should never happen, as the descriptor is a property of + a class, which won't be deleted. + """ action = self.action_ref() - assert action is not None, "The action for an `Invocation` has been deleted!" + if action is None: # pragma: no cover + # Action descriptors should only be deleted after the server has + # stopped, so this error should never occur. + raise RuntimeError("The action for an `Invocation` has been deleted!") return action @property @@ -264,7 +272,11 @@ def run(self) -> None: thing = self.thing kwargs = model_to_dict(self.input) - assert thing is not None + if thing is None: # pragma: no cover + # The Thing is stored as a weakref, but it will always exist + # while the server is running - this error should never + # occur. + raise RuntimeError("Cannot start an invocation without a Thing.") with self._status_lock: self._status = InvocationStatus.RUNNING From 6cb1e04868413b32c1f2e088f99dc34095564642 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:17 +0200 Subject: [PATCH 25/48] Remove a spurious assertion The action manager is added to a ThingServer during __init__ so it may never be None. The assertion is therefore not necessary. --- src/labthings_fastapi/dependencies/action_manager.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/labthings_fastapi/dependencies/action_manager.py b/src/labthings_fastapi/dependencies/action_manager.py index ada240e..dde9944 100644 --- a/src/labthings_fastapi/dependencies/action_manager.py +++ b/src/labthings_fastapi/dependencies/action_manager.py @@ -27,19 +27,17 @@ def action_manager_from_thing_server(request: Request) -> ActionManager: - """Retrieve the Action Manager from the Thing Server. + r"""Retrieve the Action Manager from the Thing Server. This is for use as a FastAPI dependency. We use the ``request`` to - access the `.ThingServer` and thus access the `.ActionManager`. + access the `.ThingServer` and thus access the `.ActionManager`\ . :param request: the FastAPI request object. This will be supplied by FastAPI when this function is used as a dependency. - :return: the `.ActionManager` object associated with our `.ThingServer`. + :return: the `.ActionManager` object associated with our `.ThingServer`\ . """ - action_manager = find_thing_server(request.app).action_manager - assert action_manager is not None - return action_manager + return find_thing_server(request.app).action_manager ActionManagerDep = Annotated[ActionManager, Depends(action_manager_from_thing_server)] From cf677fa8d840708599ec59d777de211d9e782101 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:17 +0200 Subject: [PATCH 26/48] Change assertion to exception I've excluded this from coverage: it's testing against the blocking portal being missing, in a dependency function. This dependency function is not intended to be called by users, and it is only evaluated by FastAPI while the server is running. --- src/labthings_fastapi/dependencies/blocking_portal.py | 10 +++++++++- src/labthings_fastapi/exceptions.py | 9 +++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/labthings_fastapi/dependencies/blocking_portal.py b/src/labthings_fastapi/dependencies/blocking_portal.py index a0e6bd0..2880e1d 100644 --- a/src/labthings_fastapi/dependencies/blocking_portal.py +++ b/src/labthings_fastapi/dependencies/blocking_portal.py @@ -28,6 +28,7 @@ from fastapi import Depends, Request from anyio.from_thread import BlockingPortal as RealBlockingPortal from .thing_server import find_thing_server +from ..exceptions import ServerNotRunningError def blocking_portal_from_thing_server(request: Request) -> RealBlockingPortal: @@ -41,12 +42,19 @@ def blocking_portal_from_thing_server(request: Request) -> RealBlockingPortal: :return: the `anyio.from_thread.BlockingPortal` allowing access to the `.ThingServer`\ 's event loop. + + :raises ServerNotRunningError: if the server does not have an available + blocking portal. This should not normally happen, as dependencies + are only evaluated while the server is running. """ portal = find_thing_server(request.app).blocking_portal - assert portal is not None, RuntimeError( + if portal is None: # pragma: no cover + raise ServerNotRunningError( "Could not get the blocking portal from the server." # This should never happen, as the blocking portal is added # and removed in `.ThingServer.lifecycle`. + # As dependencies are only evaluated while the server is running, + # this error should never be raised. ) return portal diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index e908884..b9614d5 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -20,6 +20,15 @@ class NotConnectedToServerError(RuntimeError): """ +class ServerNotRunningError(RuntimeError): + """The ThingServer is not running. + + This exception is called when a function assumes the ThingServer is + running, and it is not. This might be because the function needs to call + code in the async event loop. + """ + + class ReadOnlyPropertyError(AttributeError): """A property is read-only. From cfb18a89ff0830cb22593349854aa813628a6d48 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:27:17 +0200 Subject: [PATCH 27/48] Convert an assert to an exception This was guarding a weakref to the Thing: this isn't an error we expect, or an error we should handle, so I've excluded the if block from testing. Coverage is unchanged: the assert statement was equivalent to the new block, it just didn't show as uncovered if the asertion passed. --- src/labthings_fastapi/actions/__init__.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/actions/__init__.py b/src/labthings_fastapi/actions/__init__.py index 20b47dc..cdbc47f 100644 --- a/src/labthings_fastapi/actions/__init__.py +++ b/src/labthings_fastapi/actions/__init__.py @@ -170,7 +170,7 @@ def status(self) -> InvocationStatus: @property def action(self) -> ActionDescriptor: """The `.ActionDescriptor` object running in this thread. - + :raises RuntimeError: if the action descriptor has been deleted. This should never happen, as the descriptor is a property of a class, which won't be deleted. @@ -186,7 +186,10 @@ def action(self) -> ActionDescriptor: def thing(self) -> Thing: """The `.Thing` to which the action is bound, i.e. this is ``self``.""" thing = self.thing_ref() - assert thing is not None, "The `Thing` on which an action was run is missing!" + if thing is None: # pragma: no cover + # this error block is primarily for mypy: the Thing will exist as + # long as the server is running, so we should never hit this error. + raise RuntimeError("The `Thing` on which an action was run is missing!") return thing def cancel(self) -> None: From bf886841042a2b84688c7feb0a08ba54abd2c487 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:54 +0200 Subject: [PATCH 28/48] Convert assertions to exceptions. This includes a couple of extra tests to ensure the right error is raised if the functions are called prematurely. --- src/labthings_fastapi/descriptors/action.py | 16 ++++++++++++++-- src/labthings_fastapi/descriptors/endpoint.py | 3 +-- src/labthings_fastapi/exceptions.py | 12 +++++++++--- tests/test_actions.py | 15 +++++++++++++++ 4 files changed, 39 insertions(+), 7 deletions(-) diff --git a/src/labthings_fastapi/descriptors/action.py b/src/labthings_fastapi/descriptors/action.py index a61dd82..a9464ed 100644 --- a/src/labthings_fastapi/descriptors/action.py +++ b/src/labthings_fastapi/descriptors/action.py @@ -255,6 +255,10 @@ def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: :param thing: The `.Thing` to which the action is attached. Bear in mind that the descriptor may be used by more than one `.Thing`, so this can't be a property of the descriptor. + + :raises NotConnectedToServerError: if the function is run before the + ``thing`` has a ``path`` property. This is assigned when the `.Thing` + is added to a server. """ # We can't use the decorator in the usual way, because we'd need to @@ -318,7 +322,10 @@ def start_action( if hasattr(self.output_model, "media_type"): responses[200]["content"][self.output_model.media_type] = {} # Now we can add the endpoint to the app. - assert thing.path is not None, "Can't add the endpoint without thing.path!" + if thing.path is None: + raise NotConnectedToServerError( + "Can't add the endpoint without thing.path!" + ) app.post( thing.path + self.name, response_model=self.invocation_model, @@ -358,9 +365,14 @@ def action_affordance( omitted, we use the ``path`` property of the ``thing``. :return: An `.ActionAffordance` describing this action. + + :raises NotConnectedToServerError: if the function is run before the + ``thing`` has a ``path`` property. This is assigned when the `.Thing` + is added to a server. """ path = path or thing.path - assert path is not None, "Can't generate forms without a path!" + if path is None: + raise NotConnectedToServerError("Can't generate forms without a path!") forms = [ Form[ActionOp](href=path + self.name, op=[ActionOp.invokeaction]), ] diff --git a/src/labthings_fastapi/descriptors/endpoint.py b/src/labthings_fastapi/descriptors/endpoint.py index 7545569..7d681f2 100644 --- a/src/labthings_fastapi/descriptors/endpoint.py +++ b/src/labthings_fastapi/descriptors/endpoint.py @@ -16,8 +16,7 @@ from __future__ import annotations from functools import partial, wraps -from labthings_fastapi.exceptions import NotConnectedToServerError - +from ..exceptions import NotConnectedToServerError from ..utilities.introspection import get_docstring, get_summary from typing import ( diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index b9614d5..d0ea252 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -15,14 +15,20 @@ class NotConnectedToServerError(RuntimeError): This exception is called if an Action is called or a `.DataProperty` is updated on a Thing that is not - connected to a ThingServer. A server connection is needed - to manage asynchronous behaviour. + connected to a ThingServer. + + A server connection is needed to manage asynchronous behaviour. + + `.Thing` instances are also only assigned a ``path`` when they + are added to a server, so this error may be raised by functions + that implement the HTTP API if an attempt is made to construct + the API before the `.Thing` has been assigned a path. """ class ServerNotRunningError(RuntimeError): """The ThingServer is not running. - + This exception is called when a function assumes the ThingServer is running, and it is not. This might be because the function needs to call code in the async event loop. diff --git a/tests/test_actions.py b/tests/test_actions.py index 98c6949..f90ed2f 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -1,5 +1,7 @@ from fastapi.testclient import TestClient import pytest + +from labthings_fastapi.exceptions import NotConnectedToServerError from .temp_client import poll_task, get_link from labthings_fastapi.example_things import MyThing import labthings_fastapi as lt @@ -94,3 +96,16 @@ def test_openapi(): with TestClient(server.app) as client: r = client.get("/openapi.json") r.raise_for_status() + + +def test_affordance_and_fastapi_errors(mocker): + """Check that we get a sensible error if the Thing has no path. + + The thing will not have a ``path`` property before it has been added + to a server. + """ + thing = MyThing() + with pytest.raises(NotConnectedToServerError): + MyThing.anaction.add_to_fastapi(mocker.Mock(), thing) + with pytest.raises(NotConnectedToServerError): + MyThing.anaction.action_affordance(thing, None) From 57e5b408045dca661b4e0495335dfa9556d365b0 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 29/48] Ignore error on eval Eval is flagged as potentially insecure. This function shouldn't be run on user input, and already sanitises the string. `blob_type` is also in the process of being phased out in favour of subclassing Blob explicitly. --- src/labthings_fastapi/outputs/blob.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/labthings_fastapi/outputs/blob.py b/src/labthings_fastapi/outputs/blob.py index cd95b16..b9b8a80 100644 --- a/src/labthings_fastapi/outputs/blob.py +++ b/src/labthings_fastapi/outputs/blob.py @@ -591,7 +591,11 @@ class MyImageBlob(Blob): return create_model( f"{media_type.replace('/', '_')}_blob", __base__=Blob, - media_type=(eval(f"Literal[r'{media_type}']"), media_type), + media_type=(eval(f"Literal[r'{media_type}']"), media_type), # noqa: S307 + # This can't be done with `literal_eval` as that does not support subscripts. + # Basic sanitisation is done above by removing backslashes and single quotes, + # and using a raw string. However, the long term solution is to remove this + # function in favour of subclassing Blob, as recommended in the docs. ) From a2eec2bd852d83df4f86766bc8991ed0a2d208d6 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 30/48] Fix whitespace --- src/labthings_fastapi/base_descriptor.py | 2 +- .../dependencies/blocking_portal.py | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 0a370cd..1d57249 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -244,7 +244,7 @@ def name(self) -> str: The ``name`` of :ref:`wot_affordances` is used in their URL and in the :ref:`gen_docs` served by LabThings. - + :raises DescriptorNotAddedToClassError: if ``__set_name__`` has not yet been called. """ diff --git a/src/labthings_fastapi/dependencies/blocking_portal.py b/src/labthings_fastapi/dependencies/blocking_portal.py index 2880e1d..0438c37 100644 --- a/src/labthings_fastapi/dependencies/blocking_portal.py +++ b/src/labthings_fastapi/dependencies/blocking_portal.py @@ -42,7 +42,7 @@ def blocking_portal_from_thing_server(request: Request) -> RealBlockingPortal: :return: the `anyio.from_thread.BlockingPortal` allowing access to the `.ThingServer`\ 's event loop. - + :raises ServerNotRunningError: if the server does not have an available blocking portal. This should not normally happen, as dependencies are only evaluated while the server is running. @@ -50,12 +50,12 @@ def blocking_portal_from_thing_server(request: Request) -> RealBlockingPortal: portal = find_thing_server(request.app).blocking_portal if portal is None: # pragma: no cover raise ServerNotRunningError( - "Could not get the blocking portal from the server." - # This should never happen, as the blocking portal is added - # and removed in `.ThingServer.lifecycle`. - # As dependencies are only evaluated while the server is running, - # this error should never be raised. - ) + "Could not get the blocking portal from the server." + # This should never happen, as the blocking portal is added + # and removed in `.ThingServer.lifecycle`. + # As dependencies are only evaluated while the server is running, + # this error should never be raised. + ) return portal From cfb02612308ce98aeb53c8b3c39b196a8a3cc892 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 31/48] Swap assertion for exception. This will decrease coverage slightly. --- src/labthings_fastapi/outputs/mjpeg_stream.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/outputs/mjpeg_stream.py b/src/labthings_fastapi/outputs/mjpeg_stream.py index c500dce..b34d6d6 100644 --- a/src/labthings_fastapi/outputs/mjpeg_stream.py +++ b/src/labthings_fastapi/outputs/mjpeg_stream.py @@ -346,8 +346,17 @@ async def notify_new_frame(self, i: int) -> None: self.condition.notify_all() async def notify_stream_stopped(self) -> None: - """Raise an exception in any waiting tasks to signal the stream has stopped.""" - assert self._streaming is False + """Raise an exception in any waiting tasks to signal the stream has stopped. + + This should be run only when streaming has stopped, i.e. ``self._streaming`` + is ``False`` and an error will be raised if this isn't the case. + + :raises RuntimeError: if the stream is still streaming. + """ + if self._streaming is True: + raise RuntimeError( + "This function should only be called when the stream is stopped." + ) async with self.condition: self.condition.notify_all() From 6122d1256505b4918f97594ab34525f7a022522b Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 32/48] Swap assertions for exceptions, and test them. Tests are needed to stop coverage decreasing, as otherwise the exception isn't raised, and shows as uncovered. These errors shouldn't occur in normal operation. --- src/labthings_fastapi/properties.py | 15 +++++++++++++-- tests/test_property.py | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 84b6ca0..d741219 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -372,8 +372,14 @@ def add_to_fastapi(self, app: FastAPI, thing: Thing) -> None: :param app: The FastAPI application we are adding endpoints to. :param thing: The `.Thing` we are adding the endpoints for. + + :raises NotConnectedToServerError: if the `.Thing` does not have + a ``path`` set. """ - assert thing.path is not None + if thing.path is None: + raise NotConnectedToServerError( + "Can't add the endpoint without thing.path!" + ) # We can't use the decorator in the usual way, because we'd need to # annotate the type of `body` with `self.model` which is only defined # at runtime. @@ -415,9 +421,14 @@ def property_affordance( the ``path`` from ``thing``. :return: A description of the property in :ref:`wot_td` format. + :raises NotConnectedToServerError: if the `.Thing` does not have + a ``path`` set. """ path = path or thing.path - assert path is not None, "Cannot create a property affordance without a path" + if path is None: + raise NotConnectedToServerError( + "Can't create an affordance without thing.path!" + ) ops = [PropertyOp.readproperty] if not self.readonly: ops.append(PropertyOp.writeproperty) diff --git a/tests/test_property.py b/tests/test_property.py index 2ffac7f..11d8f6d 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -17,6 +17,7 @@ import pytest from labthings_fastapi import properties as tp from labthings_fastapi.base_descriptor import DescriptorAddedToClassTwiceError +from labthings_fastapi.exceptions import NotConnectedToServerError from .utilities import raises_or_is_caused_by @@ -266,3 +267,20 @@ def set_prop(self, val: bool) -> None: assert Example.prop.name == "prop" assert not isinstance(Example.set_prop, tp.FunctionalProperty) assert callable(Example.set_prop) + + +def test_premature_api_and_affordance(mocker): + """Check the right error is raised if we add to API without a path.""" + + class Example: + @tp.property + def prop(self) -> bool: + """An example getter.""" + return True + + example = Example() + + with pytest.raises(NotConnectedToServerError): + Example.prop.add_to_fastapi(mocker.Mock(), example) + with pytest.raises(NotConnectedToServerError): + Example.prop.property_affordance(example, None) From ada4d4923f70efebc753abc44248a1f74d372d94 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 33/48] Convert assertions to errors, and test. --- src/labthings_fastapi/server/__init__.py | 8 +++--- tests/test_server.py | 33 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) create mode 100644 tests/test_server.py diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index b4e015d..7062200 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -198,9 +198,8 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None]: # We attach a blocking portal to each thing, so that threaded code can # make callbacks to async code (needed for events etc.) for thing in self.things.values(): - assert thing._labthings_blocking_portal is None, ( - "Things may only ever have one blocking portal" - ) + if thing._labthings_blocking_portal is not None: + raise RuntimeError("Things may only ever have one blocking portal") thing._labthings_blocking_portal = portal # we __aenter__ and __aexit__ each Thing, which will in turn call the # synchronous __enter__ and __exit__ methods if they exist, to initialise @@ -287,6 +286,7 @@ def server_from_config(config: dict) -> ThingServer: f"specified as the class for {path}." ) from e instance = cls(*thing.get("args", {}), **thing.get("kwargs", {})) - assert isinstance(instance, Thing), f"{thing['class']} is not a Thing" + if not isinstance(instance, Thing): + raise TypeError(f"{thing['class']} is not a Thing") server.add_thing(instance, path) return server diff --git a/tests/test_server.py b/tests/test_server.py new file mode 100644 index 0000000..5cfc9dd --- /dev/null +++ b/tests/test_server.py @@ -0,0 +1,33 @@ +"""Test the ThingServer. + +Currently, this adds one trivial test for an error. + +While the server is covered by many of the other tests, it would +be helpful to have some more bottom-up unit testing in this file. +""" + +import pytest +from fastapi.testclient import TestClient +import labthings_fastapi as lt +from labthings_fastapi import server + + +def test_thing_with_blocking_portal_error(mocker): + """Test that a thing with a _labthings_blocking_portal causes an error.""" + + class Example(lt.Thing): + def __init__(self): + super().__init__() + self._labthings_blocking_portal = mocker.Mock() + + server = lt.ThingServer() + server.add_thing(Example(), "/example") + with pytest.raises(RuntimeError): + with TestClient(server.app) as client: + pass + + +def test_server_from_config_non_thing_error(): + """Test a typeerror is raised if something that's not a Thing is added.""" + with pytest.raises(TypeError): + server.server_from_config({"/thingone": {"class": "builtins:object"}}) From bba8aee4064b8366f881a005f4c6dc224626d8d6 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 34/48] Delete an unnecessary assertion. --- src/labthings_fastapi/server/cli.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/labthings_fastapi/server/cli.py b/src/labthings_fastapi/server/cli.py index d8e32ba..2e636d2 100644 --- a/src/labthings_fastapi/server/cli.py +++ b/src/labthings_fastapi/server/cli.py @@ -144,7 +144,6 @@ def serve_from_cli( config, server = None, None config = config_from_args(args) server = server_from_config(config) - assert isinstance(server, ThingServer) if dry_run: return server uvicorn.run(server.app, host=args.host, port=args.port) From 909934a3fadea08dc4fac949808e03483ff8d9bc Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 35/48] Replace an assertion with an exception, and add a test. --- src/labthings_fastapi/thing.py | 6 +++++- tests/test_settings.py | 11 +++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 3be3631..2aeb2fd 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -21,6 +21,7 @@ from pydantic import BaseModel +from .exceptions import NotConnectedToServerError from .properties import DataProperty, BaseSetting from .descriptors import ActionDescriptor from .thing_description._model import ThingDescription, NoSecurityScheme @@ -237,7 +238,10 @@ def save_settings(self) -> None: the settings file every time. """ if self._settings is not None: - assert self._setting_storage_path is not None + if self._setting_storage_path is None: + raise NotConnectedToServerError( + "The path to the settings file is not defined yet." + ) setting_dict = {} for name in self._settings.keys(): value = getattr(self, name) diff --git a/tests/test_settings.py b/tests/test_settings.py index 10434f6..dcda980 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -8,6 +8,7 @@ from fastapi.testclient import TestClient import labthings_fastapi as lt +from labthings_fastapi.exceptions import NotConnectedToServerError from .temp_client import poll_task @@ -338,6 +339,16 @@ def test_settings_dict_save(thing, server): assert json.load(file_obj) == _settings_dict(dictsetting={"c": 3}) +def test_premature_Settings_save(thing): + """Check a helpful error is raised if the settings path is missing. + + The settings path is only set when a thing is connected to a server, + so if we use an unconnected thing, we should see the error. + """ + with pytest.raises(NotConnectedToServerError): + thing.save_settings() + + def test_settings_dict_internal_update(thing, server): """Confirm settings are not saved if the internal value of a dictionary is updated From 1460aaa71fbfce63064b8cf9316084dd84387c0c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 36/48] Replace the last couple of assertions with exceptions. --- src/labthings_fastapi/thing_description/_model.py | 13 ++++++++++--- src/labthings_fastapi/utilities/__init__.py | 5 +++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/labthings_fastapi/thing_description/_model.py b/src/labthings_fastapi/thing_description/_model.py index 8c3c163..014a215 100644 --- a/src/labthings_fastapi/thing_description/_model.py +++ b/src/labthings_fastapi/thing_description/_model.py @@ -94,13 +94,20 @@ def uses_thing_context(v: ThingContextType) -> None: :param v: the ThingContextType object. """ if not isinstance(v, list): - assert v is THING_CONTEXT_URL + if v is not THING_CONTEXT_URL: + raise ValueError(f"{v} must be {THING_CONTEXT_URL}") # pragma: no cover + # excluded from coverage as this is hardcoded, so we shouldn't ever + # see the error. else: - assert ( + if not ( v[0] == THING_CONTEXT_URL or v[1] == THING_CONTEXT_URL and v[0] == THING_CONTEXT_URL_v1 - ) + ): + raise ValueError( + f"{v} must contain {THING_CONTEXT_URL}" + ) # pragma: no cover + # This is hard-coded, so is not an error we ever expect to see. ThingContext = Annotated[ diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index ca7b888..b4b1f13 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -114,9 +114,10 @@ def wrap_plain_types_in_rootmodel(model: type) -> type[BaseModel]: :return: A `pydantic` model, wrapping Python types in a ``RootModel`` if needed. """ try: # This needs to be a `try` as basic types are not classes - assert issubclass(model, BaseModel) + if not issubclass(model, BaseModel): + raise TypeError("Not a model (yet)") return model - except (TypeError, AssertionError): + except TypeError: return create_model(f"{model!r}", root=(model, ...), __base__=RootModel) From fe4c515daeeb05779fef30d718ae0ca07148f0ed Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 37/48] Enforce no assertions, and a few other linter rules. --- pyproject.toml | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 8934b8b..b154b7f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -79,13 +79,15 @@ docstring-code-format = true external = ["DOC401", "F824", "DOC101", "DOC103"] # used via flake8/pydoclint select = [ "ANN", # type annotations (ensuring they are present, complements mypy) + "ASYNC", # flake8 async rules "B", # flake8-bugbear "E", # flake8 "F", # flake8 "FAST", # FastAPI rules (mostly around dependencies) "D", # pydoclint (recommended by the developers over flake8 plugin) "DOC", # pydocstyle (limited support for sphinx style: see ignores) - "FAST" # FastAPI rules + "FAST", # FastAPI rules + "S", # flake8-bandit - notably, this disallows `assert` ] ignore = [ "D203", # incompatible with D204 @@ -104,10 +106,16 @@ preview = true [tool.ruff.lint.per-file-ignores] # Tests are currently not fully docstring-ed, we'll ignore this for now. -"tests/*" = ["D", "DOC", "ANN"] +"tests/*" = [ + "D", # Tests are not yet fully covered by docstrings + "DOC", # Tests are not yet fully covered by docstrings + "ANN", # We don't require type hints in test code. + "S101", # S101 disallows assert. That's not appropriate for tests. +] # Typing tests do have docstrings, but it's not helpful to insist on imperative # mood etc. "typing_tests/*" = ["D404", "D401"] +# The docs config file is a python file, but doesn't conform to the usual rules. "docs/source/conf.py" = ["D", "DOC", "ANN"] [tool.ruff.lint.pydocstyle] From 15904ee4dc2b6be1eb5adde43e8ef6a5c0f58891 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 38/48] Delete an unused variable. --- tests/test_server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_server.py b/tests/test_server.py index 5cfc9dd..10a2b9c 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -23,7 +23,7 @@ def __init__(self): server = lt.ThingServer() server.add_thing(Example(), "/example") with pytest.raises(RuntimeError): - with TestClient(server.app) as client: + with TestClient(server.app): pass From d5e07acce2b58e9ac803d96dc6adc29251645f3d Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:55 +0200 Subject: [PATCH 39/48] Don't catch blind Exceptions I've enabled a Ruff rule to disallow `except Exception:` and removed one instance of that from the codebase. I have left one `except Exception:` in test code (which captures exceptions in a thread for later analysis), and another in the MJPEG stream (which logs exceptions but does not crash the server if they happen in the MJPEG stream). The MJPEG code might be able to be eliminated now that streams stop more gracefully - but I don't want to mess with that until we're in a position to test it more extensively. I've swapped the `logging.error` for `logging.exception` so that the full stack trace is dumped to the log. --- pyproject.toml | 1 + src/labthings_fastapi/descriptors/action.py | 28 ++++++++----------- src/labthings_fastapi/outputs/mjpeg_stream.py | 7 +++-- tests/test_server_cli.py | 5 +++- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index b154b7f..a23e94b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -81,6 +81,7 @@ select = [ "ANN", # type annotations (ensuring they are present, complements mypy) "ASYNC", # flake8 async rules "B", # flake8-bugbear + "BLE", # don't catch Exception "E", # flake8 "F", # flake8 "FAST", # FastAPI rules (mostly around dependencies) diff --git a/src/labthings_fastapi/descriptors/action.py b/src/labthings_fastapi/descriptors/action.py index a9464ed..79e6190 100644 --- a/src/labthings_fastapi/descriptors/action.py +++ b/src/labthings_fastapi/descriptors/action.py @@ -204,23 +204,19 @@ def emit_changed_event(self, obj: Thing, status: str) -> None: :raise NotConnectedToServerError: if the Thing calling the action is not connected to a server with a running event loop. """ - try: - runner = get_blocking_portal(obj) - if not runner: - thing_name = obj.__class__.__name__ - msg = ( - f"Cannot emit action changed event. Is {thing_name} connected to " - "a running server?" - ) - raise NotConnectedToServerError(msg) - runner.start_task_soon( - self.emit_changed_event_async, - obj, - status, + runner = get_blocking_portal(obj) + if not runner: + thing_name = obj.__class__.__name__ + msg = ( + f"Cannot emit action changed event. Is {thing_name} connected to " + "a running server?" ) - except Exception: - # TODO: in the unit test, the get_blocking_portal throws exception - ... + raise NotConnectedToServerError(msg) + runner.start_task_soon( + self.emit_changed_event_async, + obj, + status, + ) async def emit_changed_event_async(self, obj: Thing, value: Any) -> None: """Notify subscribers that the action status has changed. diff --git a/src/labthings_fastapi/outputs/mjpeg_stream.py b/src/labthings_fastapi/outputs/mjpeg_stream.py index b34d6d6..740c037 100644 --- a/src/labthings_fastapi/outputs/mjpeg_stream.py +++ b/src/labthings_fastapi/outputs/mjpeg_stream.py @@ -288,8 +288,11 @@ async def frame_async_generator(self) -> AsyncGenerator[bytes, None]: yield frame except StopAsyncIteration: break - except Exception as e: - logging.error(f"Error in stream: {e}, stream stopped") + except Exception as e: # noqa: BLE001 + # It's important that errors in the stream don't crash the server. + # This may be something we can remove in the future, now streams stop + # more elegantly. However, it will require careful testing.f + logging.exception(f"Error in stream: {e}, stream stopped") return async def mjpeg_stream_response(self) -> MJPEGStreamResponse: diff --git a/tests/test_server_cli.py b/tests/test_server_cli.py index e2d63f1..06a3d97 100644 --- a/tests/test_server_cli.py +++ b/tests/test_server_cli.py @@ -20,7 +20,10 @@ def monitored_target(target, conn, *args, **kwargs): try: ret = target(*args, **kwargs) conn.send(("success", ret)) - except Exception as e: + except Exception as e: # noqa: BLE001 + # this is test code, and any exceptions should be inspected + # by the calling function. Catching Exception is therefore + # OK in this case. conn.send(("exception", e)) except SystemExit as e: conn.send(("exit", e)) From 41bb168d6cb404b029133ef67eaa7d07e367bd17 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:56 +0200 Subject: [PATCH 40/48] Properly mock `Thing.path` when needed. Test code that will fail without a path should mock one. I've added a path in 3 places where this was required in the test suite. --- tests/test_numpy_type.py | 2 ++ tests/test_property.py | 2 ++ tests/test_thing.py | 1 + 3 files changed, 5 insertions(+) diff --git a/tests/test_numpy_type.py b/tests/test_numpy_type.py index a99db29..b9b1d0c 100644 --- a/tests/test_numpy_type.py +++ b/tests/test_numpy_type.py @@ -70,6 +70,8 @@ def action_with_arrays(self, a: NDArray) -> NDArray: def test_thing_description(): thing = MyNumpyThing() + # We must mock a path, or it can't generate a Thing Description. + thing.path = "/mynumpything" assert thing.validate_thing_description() is None diff --git a/tests/test_property.py b/tests/test_property.py index 11d8f6d..81118a1 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -273,6 +273,8 @@ def test_premature_api_and_affordance(mocker): """Check the right error is raised if we add to API without a path.""" class Example: + path = None # this is supplied by `lt.Thing` but we're not subclassing. + @tp.property def prop(self) -> bool: """An example getter.""" diff --git a/tests/test_thing.py b/tests/test_thing.py index 86df025..88c2e10 100644 --- a/tests/test_thing.py +++ b/tests/test_thing.py @@ -6,6 +6,7 @@ def test_td_validates(): """This will raise an exception if it doesn't validate OK""" thing = MyThing() + thing.path = "/mything" # can't generate a TD without a path assert thing.validate_thing_description() is None From 07c92e25c28bfdf0d89e4edfcaadd7aa6fa2eb3b Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:56 +0200 Subject: [PATCH 41/48] Fix failing tests of the server Exceptions raised when setting up things were being wrapped in an ExceptionGroup: this is fixed by using pytest.RasisesGroup, but that required updating pytest. The test that checked for errors when a non-Thing was added to the server was failing, because the config was invalid. I've fixed it, but we really ought to use a model here. I've also renamed the import to avoid confusion with `server`, so we now import the server module as `ts`. --- dev-requirements.txt | 19 ++++++++++++++++++- pyproject.toml | 2 +- src/labthings_fastapi/server/__init__.py | 3 ++- tests/test_server.py | 22 +++++++++++++++++----- 4 files changed, 38 insertions(+), 8 deletions(-) diff --git a/dev-requirements.txt b/dev-requirements.txt index 7968c72..1b210b6 100644 --- a/dev-requirements.txt +++ b/dev-requirements.txt @@ -55,6 +55,10 @@ email-validator==2.2.0 # via # fastapi # pydantic +exceptiongroup==1.3.0 + # via + # anyio + # pytest fastapi==0.116.1 # via labthings-fastapi (pyproject.toml) fastapi-cli==0.0.8 @@ -160,9 +164,10 @@ pyflakes==3.4.0 pygments==2.19.2 # via # flake8-rst-docstrings + # pytest # rich # sphinx -pytest==7.4.4 +pytest==8.4.1 # via # labthings-fastapi (pyproject.toml) # pytest-cov @@ -241,6 +246,14 @@ sphinxcontrib-serializinghtml==2.0.0 # via sphinx starlette==0.47.1 # via fastapi +tomli==2.2.1 + # via + # coverage + # flake8-pyproject + # mypy + # pydoclint + # pytest + # sphinx typer==0.16.0 # via # fastapi-cli @@ -251,16 +264,20 @@ typing-extensions==4.14.1 # via # labthings-fastapi (pyproject.toml) # anyio + # astroid + # exceptiongroup # fastapi # mypy # pydantic # pydantic-core # pydantic-extra-types # referencing + # rich # rich-toolkit # starlette # typer # typing-inspection + # uvicorn typing-inspection==0.4.1 # via pydantic-settings ujson==5.10.0 diff --git a/pyproject.toml b/pyproject.toml index a23e94b..939e362 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -25,7 +25,7 @@ dependencies = [ [project.optional-dependencies] dev = [ - "pytest>=7.4.0, <8", + "pytest>=8.4.0", "pytest-cov", "pytest-mock", "mypy>=1.6.1, <2", diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 7062200..e0700ad 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -260,7 +260,7 @@ def thing_paths(request: Request) -> Mapping[str, str]: def server_from_config(config: dict) -> ThingServer: - """Create a ThingServer from a configuration dictionary. + r"""Create a ThingServer from a configuration dictionary. This function creates a `.ThingServer` and adds a number of `.Thing` instances from a configuration dictionary. @@ -273,6 +273,7 @@ def server_from_config(config: dict) -> ThingServer: :raise ImportError: if a Thing could not be loaded from the specified object reference. + :raise TypeError: if a class is specified that does not subclass `.Thing`\ . """ server = ThingServer(config.get("settings_folder", None)) for path, thing in config.get("things", {}).items(): diff --git a/tests/test_server.py b/tests/test_server.py index 10a2b9c..f3edb8f 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -9,11 +9,23 @@ import pytest from fastapi.testclient import TestClient import labthings_fastapi as lt -from labthings_fastapi import server +from labthings_fastapi import server as ts def test_thing_with_blocking_portal_error(mocker): - """Test that a thing with a _labthings_blocking_portal causes an error.""" + """Test that a thing with a _labthings_blocking_portal causes an error. + + The blocking portal is added when the server starts. If one is there already, + this is an error and the server should fail to start. + + As this ends up in an async context manager, the exception will be wrapped + in an ExceptionGroup, hence the slightly complicated code to test the exception. + + This is not an error condition that we expect to happen often. Handling + it more elegantly would result in enough additional code that the burden of + maintaining and testing that code outweighs the benefit of a more elegant + error message. + """ class Example(lt.Thing): def __init__(self): @@ -22,12 +34,12 @@ def __init__(self): server = lt.ThingServer() server.add_thing(Example(), "/example") - with pytest.raises(RuntimeError): + with pytest.RaisesGroup(pytest.RaisesExc(RuntimeError, match="blocking portal")): with TestClient(server.app): pass def test_server_from_config_non_thing_error(): """Test a typeerror is raised if something that's not a Thing is added.""" - with pytest.raises(TypeError): - server.server_from_config({"/thingone": {"class": "builtins:object"}}) + with pytest.raises(TypeError, match="not a Thing"): + ts.server_from_config({"things": {"/thingone": {"class": "builtins:object"}}}) From a19f97b520737cbd3a9ee7413d4d7ffd7ab42d4e Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:56 +0200 Subject: [PATCH 42/48] Add newly-raised exceptions to docstrings --- src/labthings_fastapi/actions/__init__.py | 7 ++++++- src/labthings_fastapi/server/__init__.py | 4 ++++ src/labthings_fastapi/thing.py | 5 +++++ src/labthings_fastapi/thing_description/_model.py | 2 ++ 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/labthings_fastapi/actions/__init__.py b/src/labthings_fastapi/actions/__init__.py index cdbc47f..d1ca384 100644 --- a/src/labthings_fastapi/actions/__init__.py +++ b/src/labthings_fastapi/actions/__init__.py @@ -184,7 +184,10 @@ def action(self) -> ActionDescriptor: @property def thing(self) -> Thing: - """The `.Thing` to which the action is bound, i.e. this is ``self``.""" + """The `.Thing` to which the action is bound, i.e. this is ``self``. + + :raises RuntimeError: if the Thing no longer exists. + """ thing = self.thing_ref() if thing is None: # pragma: no cover # this error block is primarily for mypy: the Thing will exist as @@ -261,6 +264,8 @@ def run(self) -> None: stored. The status is then set to ERROR and the thread terminates. See `.Invocation.status` for status values. + + :raises RuntimeError: if there is no Thing associated with the invocation. """ # self.action evaluates to an ActionDescriptor. This confuses mypy, # which thinks we are calling ActionDescriptor.__get__. diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index e0700ad..34f8add 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -192,6 +192,10 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None]: :param app: The FastAPI application wrapped by the server. :yield: no value. The FastAPI application will serve requests while this function yields. + + :raises RuntimeError: if a `.Thing` already has a blocking portal attached. + This should never happen, and suggests the server is being used to + serve a `.Thing` that is already being served elsewhere. """ async with BlockingPortal() as portal: self.blocking_portal = portal diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 2aeb2fd..0c8e7c1 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -236,6 +236,11 @@ def save_settings(self) -> None: This is called whenever a setting is updated. All settings are written to the settings file every time. + + :raises NotConnectedToServerError: if there is no settings file path set. + This is set when the `.Thing` is connected to a `.ThingServer` so + most likely we are trying to save settings before we are attached + to a server. """ if self._settings is not None: if self._setting_storage_path is None: diff --git a/src/labthings_fastapi/thing_description/_model.py b/src/labthings_fastapi/thing_description/_model.py index 014a215..4a3f951 100644 --- a/src/labthings_fastapi/thing_description/_model.py +++ b/src/labthings_fastapi/thing_description/_model.py @@ -92,6 +92,8 @@ def uses_thing_context(v: ThingContextType) -> None: This refers to the ``@context`` property. :param v: the ThingContextType object. + + :raises ValueError: if the URL is not correct. """ if not isinstance(v, list): if v is not THING_CONTEXT_URL: From a2332708fd2884d61be0b773652bafc68ebd0231 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:56 +0200 Subject: [PATCH 43/48] Avoid raising an exception that's immediately handled This previously used an `assert` so that any types that weren't models were handled in the `except` block. I've changed this so that plain types are wrapped in a return statement after the except: block, which I think is clearer than the previous structure. --- src/labthings_fastapi/utilities/__init__.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index b4b1f13..fb94d78 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -114,11 +114,11 @@ def wrap_plain_types_in_rootmodel(model: type) -> type[BaseModel]: :return: A `pydantic` model, wrapping Python types in a ``RootModel`` if needed. """ try: # This needs to be a `try` as basic types are not classes - if not issubclass(model, BaseModel): - raise TypeError("Not a model (yet)") - return model + if issubclass(model, BaseModel): + return model except TypeError: - return create_model(f"{model!r}", root=(model, ...), __base__=RootModel) + pass # some plain types aren't classes and that's OK - they still get wrapped. + return create_model(f"{model!r}", root=(model, ...), __base__=RootModel) def model_to_dict(model: Optional[BaseModel]) -> Dict[str, Any]: From 30f44930df788accca419577f271a9721a909d21 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:28:56 +0200 Subject: [PATCH 44/48] Enable flake8-comprehension ruleset --- pyproject.toml | 1 + tests/test_property.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 939e362..918152c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -82,6 +82,7 @@ select = [ "ASYNC", # flake8 async rules "B", # flake8-bugbear "BLE", # don't catch Exception + "C4", # flake8-comprehension rules "E", # flake8 "F", # flake8 "FAST", # FastAPI rules (mostly around dependencies) diff --git a/tests/test_property.py b/tests/test_property.py index 81118a1..96ceef6 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -218,7 +218,7 @@ class Example: # Check the property appears at the expected place entry = openapi["paths"]["/example/prop"] # Check it declares the right methods - assert set(entry.keys()) == set(["get", "put"]) + assert set(entry.keys()) == {"get", "put"} def test_baseproperty_set_error(): From 903f9e5b92083a71e90dd097faf04b9bebe61648 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 14:49:21 +0200 Subject: [PATCH 45/48] Rename TestThing. TestThing causes a warning in pytest: I've renamed to avoid the confusion. --- tests/test_settings.py | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/tests/test_settings.py b/tests/test_settings.py index dcda980..f31e329 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -12,7 +12,7 @@ from .temp_client import poll_task -class TestThing(lt.Thing): +class ThingWithSettings(lt.Thing): """A test `.Thing` with some settings and actions.""" def __init__(self) -> None: @@ -87,12 +87,14 @@ def toggle_boolsetting_from_thread(self): t.start() -TestThingClientDep = lt.deps.direct_thing_client_dependency(TestThing, "/thing/") -TestThingDep = lt.deps.raw_thing_dependency(TestThing) +ThingWithSettingsClientDep = lt.deps.direct_thing_client_dependency( + ThingWithSettings, "/thing/" +) +ThingWithSettingsDep = lt.deps.raw_thing_dependency(ThingWithSettings) class ClientThing(lt.Thing): - """This Thing attempts to set read-only settings on TestThing. + """This Thing attempts to set read-only settings on ThingWithSettings. Read-only settings may not be set by DirectThingClient wrappers, which is what this class tests. @@ -101,7 +103,7 @@ class ClientThing(lt.Thing): @lt.thing_action def set_localonlysetting( self, - client: TestThingClientDep, + client: ThingWithSettingsClientDep, val: str, ): """Attempt to set a setting with a DirectThingClient.""" @@ -110,7 +112,7 @@ def set_localonlysetting( @lt.thing_action def set_localonly_boolsetting( self, - client: TestThingClientDep, + client: ThingWithSettingsClientDep, val: bool, ): """Attempt to set a setting with a DirectThingClient. @@ -123,7 +125,7 @@ def set_localonly_boolsetting( @lt.thing_action def directly_set_localonlysetting( self, - test_thing: TestThingDep, + test_thing: ThingWithSettingsDep, val: str, ): """Attempt to set a setting directly.""" @@ -132,7 +134,7 @@ def directly_set_localonlysetting( @lt.thing_action def directly_set_localonly_boolsetting( self, - test_thing: TestThingDep, + test_thing: ThingWithSettingsDep, val: bool, ): """Attempt to set a setting directly. @@ -174,7 +176,7 @@ def _settings_dict( @pytest.fixture def thing(): - return TestThing() + return ThingWithSettings() @pytest.fixture From cf815f6c737b350d7f360ed5c767c84a89d432bc Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 20:18:51 +0200 Subject: [PATCH 46/48] Improve comments in tests. I was looking for the best place to add a test, and noticed the comments/docstrings here needed improvement. --- tests/test_blob_output.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/test_blob_output.py b/tests/test_blob_output.py index e3bb056..749bc42 100644 --- a/tests/test_blob_output.py +++ b/tests/test_blob_output.py @@ -77,23 +77,27 @@ def test_blob_type(): def test_blob_creation(): """Check that blobs can be created in three ways""" TEXT = b"Test input" + # Create a blob from a file in a temporary directory td = TemporaryDirectory() with open(os.path.join(td.name, "test_input"), "wb") as f: f.write(TEXT) + # This creates the blob from only a file. It won't preserve + # the temporary directory. blob = TextBlob.from_file(os.path.join(td.name, "test_input")) assert blob.content == TEXT + # This will preserve the temporary directory, as it's + # saved in the underlying BlobData object (asserted below). blob = TextBlob.from_temporary_directory(td, "test_input") assert blob.content == TEXT assert blob.data._temporary_directory is td + + # Finally, check we can make a blob from a bytes object, no file. blob = TextBlob.from_bytes(TEXT) assert blob.content == TEXT def test_blob_output_client(): - """Test that a Thing can depend on another Thing - - This uses the internal thing client mechanism. - """ + """Test that blob outputs work as expected when used over HTTP.""" server = lt.ThingServer() server.add_thing(ThingOne(), "/thing_one") with TestClient(server.app) as client: @@ -102,13 +106,13 @@ def test_blob_output_client(): def test_blob_output_direct(): - """This should mirror `test_blob_output_inserver` but with helpful errors""" + """Check blob outputs work correctly when we use a Thing directly in Python.""" thing = ThingOne() check_actions(thing) def test_blob_output_inserver(): - """Test that the blob output works the same when used directly""" + """Test that the blob output works the same when used via a DirectThingClient.""" server = lt.ThingServer() server.add_thing(ThingOne(), "/thing_one") server.add_thing(ThingTwo(), "/thing_two") @@ -131,7 +135,11 @@ def check_blob(output, expected_content: bytes): def check_actions(thing): - """Check that both action_one and action_two work""" + """Check that both action_one and action_two work. + + This should work if called on a ThingOne directly, or a DirectThingClient, + or an HTTP ThingClient. + """ for action in (thing.action_one, thing.action_two, thing.action_three): output = action() check_blob(output, ThingOne.ACTION_ONE_RESULT) @@ -157,6 +165,3 @@ def test_blob_input(): # Check that the same thing works on the server side tc2 = lt.ThingClient.from_url("/thing_two/", client=client) assert tc2.check_passthrough() is True - - -# TODO: check that the stub serialiser isn't being used From 486c66a8260a182a0bfc8c335726d6b91c8b6ffb Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 19:30:53 +0100 Subject: [PATCH 47/48] Add tests for errors when retrieving action outputs. Previous tests didn't check what happened for bad action IDs or actions with no output. This is now tested. --- tests/test_actions.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/test_actions.py b/tests/test_actions.py index f90ed2f..27e1341 100644 --- a/tests/test_actions.py +++ b/tests/test_actions.py @@ -1,3 +1,4 @@ +import uuid from fastapi.testclient import TestClient import pytest @@ -81,15 +82,43 @@ def action_with_varargs(self, *args) -> None: def test_action_output(): + """Test that an action's output may be retrieved directly. + + This tests the /action_invocation/{id}/output endpoint, including + some error conditions (not found/output not available). + """ with TestClient(server.app) as client: + # Start an action and wait for it to complete r = client.post("/thing/make_a_dict", json={}) r.raise_for_status() invocation = poll_task(client, r.json()) assert invocation["status"] == "completed" assert invocation["output"] == {"key": "value"} + # Retrieve the output directly and check it matches r = client.get(get_link(invocation, "output")["href"]) assert r.json() == {"key": "value"} + # Test an action that doesn't have an output + r = client.post("/thing/action_without_arguments", json={}) + r.raise_for_status() + invocation = poll_task(client, r.json()) + assert invocation["status"] == "completed" + assert invocation["output"] is None + + # If the output is None, retrieving it directly should fail + r = client.get(get_link(invocation, "output")["href"]) + assert r.status_code == 503 + + # Repeat the last check, using a manually generated URL + # (mostly to check the manually generated URL is valid, + # so the next test can be trusted). + r = client.get(f"/action_invocation/{invocation['id']}/output") + assert r.status_code == 404 + + # Test an output on a non-existent invocation + r = client.get(f"/action_invocation/{uuid.uuid4()}/output") + assert r.status_code == 404 + def test_openapi(): """Check the OpenAPI docs are generated OK""" From 8187a920bc3c87acebb27df76de2fc1966056f4c Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 25 Aug 2025 21:13:55 +0100 Subject: [PATCH 48/48] Test a couple of unchecked code paths in utilities. This adds a proper unit test for model_to_dict that checks all code paths. --- tests/test_utilities.py | 57 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 tests/test_utilities.py diff --git a/tests/test_utilities.py b/tests/test_utilities.py new file mode 100644 index 0000000..5918a22 --- /dev/null +++ b/tests/test_utilities.py @@ -0,0 +1,57 @@ +"""Tests for the utilities module. + +This should grow over time. It would also be nice to figure out a way of +checking that the utilities module is covered by other tests - because if +it's not, it may mean those utility functions should be removed as they +are not used. +""" + +from pydantic import BaseModel, RootModel +import pytest +from labthings_fastapi import utilities +from labthings_fastapi.utilities.introspection import EmptyObject + + +class OptionalInt(RootModel): + """A RootModel for a type that allows None.""" + + root: int | None = None + + +class EmptyRootModel(RootModel): + """A RootModel that may contain an EmptyObject.""" + + root: EmptyObject | str + + +class MyModel(BaseModel): + a: int = 1 + b: str = "two" + c: OptionalInt = 1 + + +def test_model_to_dict(): + """Check we can non-recursively convert Pydantic models to dictionaries.""" + assert utilities.model_to_dict(None) == {} + assert utilities.model_to_dict(EmptyObject()) == {} + + # A meaningful object should turn into a dictionary, but sub-models + # should stay as models. + d1 = utilities.model_to_dict(MyModel(a=5, b="b", c=None)) + assert set(d1.keys()) == {"a", "b", "c"} + assert d1["a"] == 5 + assert d1["b"] == "b" + # c should **not** be None, as the conversion isn't recursive, it should + # be a model instance. + assert isinstance(d1["c"], OptionalInt) + assert d1["c"].root is None + + # RootModels that evaluate to None are allowed + assert utilities.model_to_dict(OptionalInt(None)) == {} + assert utilities.model_to_dict(EmptyRootModel(EmptyObject())) == {} + + # RootModels that don't evaluate to None are not allowed + with pytest.raises(ValueError): + utilities.model_to_dict(EmptyRootModel("foo")) + with pytest.raises(ValueError): + utilities.model_to_dict(OptionalInt(0))