From 2e13f2cbd81fe1978b4bdc49540651abd6c1a598 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 12 Sep 2025 22:11:56 +0100 Subject: [PATCH 01/29] Refactor field-like typing logic into base_descriptor. I'm going to re-use the field-like type annotations for thing_connection, so it makes sense to move this into BaseDescriptor. --- src/labthings_fastapi/base_descriptor.py | 89 +++++++++++++++++++ src/labthings_fastapi/exceptions.py | 31 +++++++ src/labthings_fastapi/properties.py | 107 +++++------------------ 3 files changed, 141 insertions(+), 86 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index a9fdf7a..a0c0723 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -7,15 +7,18 @@ from __future__ import annotations import ast +import builtins import inspect from itertools import pairwise import textwrap from typing import overload, Generic, Mapping, TypeVar, TYPE_CHECKING from types import MappingProxyType +import typing from weakref import WeakKeyDictionary from typing_extensions import Self from .utilities.introspection import get_docstring, get_summary +from .exceptions import MissingTypeError, InconsistentTypeError if TYPE_CHECKING: from .thing import Thing @@ -169,6 +172,7 @@ class Example: def __init__(self) -> None: """Initialise a BaseDescriptor.""" + super().__init__() self._name: str | None = None self._title: str | None = None self._description: str | None = None @@ -353,6 +357,91 @@ def instance_get(self, obj: Thing) -> Value: ) +class FieldTypedBaseDescriptor(Generic[Value], BaseDescriptor[Value]): + """A BaseDescriptor that determines its type like a dataclass field.""" + + def __init__(self) -> None: + """Initialise the FieldTypedBaseDescriptor.""" + self._type: type | None = None # Will be set in __set_name__ + + def __set_name__(self, owner: type[Thing], name: str) -> None: + """Take note of the name and type. + + This function is where we determine the type of the property. It may + be specified in two ways: either by subscripting the descriptor + or by annotating the attribute. This example is for ``DataProperty`` + as this class is not intended to be used directly. + + .. code-block:: python + + class MyThing(Thing): + subscripted_property = DataProperty[int](0) + annotated_property: int = DataProperty(0) + + The second form often works better with autocompletion, though it + is usually called via a function to avoid type checking errors. + + Neither form allows us to access the type during ``__init__``, which + is why we find the type here. If there is a problem, exceptions raised + will appear to come from the class definition, so it's important to + include the name of the attribute. + + See :ref:`descriptors` for links to the Python docs about when + this function is called. + + :param owner: the `.Thing` subclass to which we are being attached. + :param name: the name to which we have been assigned. + + :raises InconsistentTypeError: if the type is specified twice and + the two types are not identical. + :raises MissingTypeError: if no type hints have been given. + """ + # Call BaseDescriptor so we remember the name + super().__set_name__(owner, name) + + # Check for type subscripts + if hasattr(self, "__orig_class__"): + # We have been instantiated with a subscript, e.g. BaseProperty[int]. + # + # __orig_class__ is set on generic classes when they are instantiated + # with a subscripted type. + self._type = typing.get_args(self.__orig_class__)[0] + + # Check for annotations on the parent class + annotations = typing.get_type_hints(owner, include_extras=True) + field_annotation = annotations.get(name, None) + if field_annotation is not None: + # We have been assigned to an annotated class attribute, e.g. + # myprop: int = BaseProperty(0) + if self._type is not None and self._type != field_annotation: + raise InconsistentTypeError( + f"Property {name} on {owner} has conflicting types.\n\n" + f"The field annotation of {field_annotation} conflicts " + f"with the inferred type of {self._type}." + ) + self._type = field_annotation + if self._type is None: + raise MissingTypeError( + f"No type hint was found for property {name} on {owner}." + ) + + @builtins.property + def value_type(self) -> type[Value]: + """The type of this descriptor's value. + + This is only available after ``__set_name__`` has been called, which happens + at the end of the class definition. If it is called too early, a + `.DescriptorNotAddedToClassError` will be raised. + + :return: the type of the descriptor's value. + :raises MissingTypeError: if the type is None or not specified. + """ + self.assert_set_name_called() + if self._type is None: + raise MissingTypeError(f"No type hint was found for property {self.name}. ") + return self._type + + # get_class_attribute_docstrings is a relatively expensive function that # will be called potentially quite a few times on the same class. It will # return the same result each time (because it depends only on the source diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 2f81bfe..40d16bd 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -51,3 +51,34 @@ class PropertyNotObservableError(RuntimeError): observable: functional properties (using a getter/setter) may not be observed. """ + + +class InconsistentTypeError(TypeError): + """Different type hints have been given for a descriptor. + + Some descriptors in LabThings, particularly `.DataProperty` and `.ThingConnection` + may have their type specified in different ways. If multiple type hints are + provided, they must match. See `.property` for more details. + """ + + +class MissingTypeError(TypeError): + """No type hints have been given for a descriptor that requires a type. + + Every property and thing connection should have a type hint, + There are different ways of providing these type hints. + This error indicates that no type hint was found. + + See documentation for `.property` and `.thing_connection` for more details. + """ + + +class ThingNotConnectedError(RuntimeError): + """ThingConnections have not yet been set up. + + This error is raised if a ThingConnection is accessed before the `.Thing` has + been supplied by the LabThings server. This usually happens because either + the `.Thing` is being used without a server (in which case the attribute + should be mocked), or because it has been accessed before ``__enter__`` + has been called. + """ diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index c24d369..104ed46 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -58,7 +58,6 @@ class attribute. Documentation is in strings immediately following the TYPE_CHECKING, ) from typing_extensions import Self -import typing from weakref import WeakSet from fastapi import Body, FastAPI @@ -73,10 +72,11 @@ class attribute. Documentation is in strings immediately following the ) from .utilities import labthings_data, wrap_plain_types_in_rootmodel from .utilities.introspection import return_type -from .base_descriptor import BaseDescriptor +from .base_descriptor import BaseDescriptor, FieldTypedBaseDescriptor from .exceptions import ( NotConnectedToServerError, ReadOnlyPropertyError, + MissingTypeError, ) if TYPE_CHECKING: @@ -115,23 +115,6 @@ class MissingDefaultError(ValueError): """ -class InconsistentTypeError(TypeError): - """Different type hints have been given for a property. - - Every property should have a type hint, which may be provided in a few - different ways. If multiple type hints are provided, they must match. - See `.property` for more details. - """ - - -class MissingTypeError(TypeError): - """No type hints have been given for a property. - - Every property should have a type hint, which may be provided in a few - different ways. This error indicates that no type hint was found. - """ - - Value = TypeVar("Value") if TYPE_CHECKING: # It's hard to type check methods, because the type of ``self`` @@ -333,7 +316,6 @@ class BaseProperty(BaseDescriptor[Value], Generic[Value]): def __init__(self) -> None: """Initialise a BaseProperty.""" super().__init__() - self._type: type | None = None self._model: type[BaseModel] | None = None self.readonly: bool = False @@ -341,12 +323,10 @@ def __init__(self) -> None: def value_type(self) -> type[Value]: """The type of this descriptor's value. - :raises MissingTypeError: if the type has not been set. + :raises NotImplementedError: because this method must be overridden. :return: the type of the descriptor's value. """ - if self._type is None: - raise MissingTypeError("This property does not have a valid type.") - return self._type + raise NotImplementedError @builtins.property def model(self) -> type[BaseModel]: @@ -471,7 +451,9 @@ def __set__(self, obj: Thing, value: Any) -> None: ) -class DataProperty(BaseProperty[Value], Generic[Value]): +class DataProperty( + FieldTypedBaseDescriptor[Value], BaseProperty[Value], Generic[Value] +): """A Property descriptor that acts like a regular variable. `.DataProperty` descriptors remember their value, and can be read and @@ -530,67 +512,6 @@ def __init__( default=default, default_factory=default_factory ) self.readonly = readonly - self._type: type | None = None # Will be set in __set_name__ - - def __set_name__(self, owner: type[Thing], name: str) -> None: - """Take note of the name and type. - - This function is where we determine the type of the property. It may - be specified in two ways: either by subscripting ``DataProperty`` - or by annotating the attribute: - - .. code-block:: python - - class MyThing(Thing): - subscripted_property = DataProperty[int](0) - annotated_property: int = DataProperty(0) - - The second form often works better with autocompletion, though it is - preferred to use `.property` for consistent naming. - - Neither form allows us to access the type during ``__init__``, which - is why we find the type here. If there is a problem, exceptions raised - will appear to come from the class definition, so it's important to - include the name of the attribute. - - See :ref:`descriptors` for links to the Python docs about when - this function is called. - - :param owner: the `.Thing` subclass to which we are being attached. - :param name: the name to which we have been assigned. - - :raises InconsistentTypeError: if the type is specified twice and - the two types are not identical. - :raises MissingTypeError: if no type hints have been given. - """ - # Call BaseDescriptor so we remember the name - super().__set_name__(owner, name) - - # Check for type subscripts - if hasattr(self, "__orig_class__"): - # We have been instantiated with a subscript, e.g. BaseProperty[int]. - # - # __orig_class__ is set on generic classes when they are instantiated - # with a subscripted type. - self._type = typing.get_args(self.__orig_class__)[0] - - # Check for annotations on the parent class - annotations = typing.get_type_hints(owner, include_extras=True) - field_annotation = annotations.get(name, None) - if field_annotation is not None: - # We have been assigned to an annotated class attribute, e.g. - # myprop: int = BaseProperty(0) - if self._type is not None and self._type != field_annotation: - raise InconsistentTypeError( - f"Property {name} on {owner} has conflicting types.\n\n" - f"The field annotation of {field_annotation} conflicts " - f"with the inferred type of {self._type}." - ) - self._type = field_annotation - if self._type is None: - raise MissingTypeError( - f"No type hint was found for property {name} on {owner}." - ) @builtins.property def value_type(self) -> type[Value]: # noqa: DOC201 @@ -701,9 +622,23 @@ def __init__( super().__init__() self._fget: ValueGetter = fget self._type = return_type(self._fget) + if self._type is None: + msg = ( + f"{fget} does not have a valid type. " + "Return type annotations are required for property getters." + ) + raise MissingTypeError(msg) self._fset: ValueSetter | None = None self.readonly: bool = True + @builtins.property + def value_type(self) -> type[Value]: + """The type of this descriptor's value. + + :return: the type of the descriptor's value. + """ + return self._type + @builtins.property def fget(self) -> ValueGetter: # noqa: DOC201 """The getter function.""" From 78a9720ab132a698e1ebebeeea4409b679b3faf7 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Fri, 12 Sep 2025 22:11:56 +0100 Subject: [PATCH 02/29] First implementation of ThingConnection. This implements a descriptor that will be used to connect Things together. It will need logic in the ThingServer before it is useful. --- src/labthings_fastapi/thing_connection.py | 107 ++++++++++++++++++++++ 1 file changed, 107 insertions(+) create mode 100644 src/labthings_fastapi/thing_connection.py diff --git a/src/labthings_fastapi/thing_connection.py b/src/labthings_fastapi/thing_connection.py new file mode 100644 index 0000000..319e2df --- /dev/null +++ b/src/labthings_fastapi/thing_connection.py @@ -0,0 +1,107 @@ +r"""Facilitate connections between Things. + +It is often desirable for two Things in the same server to be able to communicate. +In order to do this in a nicely typed way that is easy to test and inspect, +LabThings-FastAPI provides the `.thing_connection`\ . This allows a `.Thing` +to declare that it depends on another `.Thing` being present, and provides a way for +the server to automatically connect the two when the server is set up. + +Thing connections are set up **after** all the `.Thing` instances are initialised. +This means you should not rely on them during initialisation: if you attempt to +access a connection before it is provided, it will raise an exception. The +advantage of making connections after initialisation is that circular connections +are not a problem: Thing `a` may depend on Thing `b` and vice versa. + +As with properties, thing connections will usually be declared using the function +`.thing_connection` rather than the descriptor directly. This allows them to be +typed and documented on the class, i.e. + +.. code-block:: python + + import labthings_fastapi as lt + + + class ThingA(lt.Thing): + "A class that doesn't do much." + + @lt.action + def say_hello(self) -> str: + "A canonical example function." + return "Hello world." + + + class ThingB(lt.Thing): + "A class that relies on ThingA." + + thing_a: ThingA = lt.thing_connection() + + @lt.action + def say_hello(self) -> str: + "I'm too lazy to say hello, ThingA does it for me." + return self.thing_a.say_hello() +""" + +from typing import Generic, TypeVar, TYPE_CHECKING +from weakref import WeakKeyDictionary, ref +from .base_descriptor import FieldTypedBaseDescriptor +from .exceptions import ThingNotConnectedError + +if TYPE_CHECKING: + from .thing import Thing + + +ThingSubclass = TypeVar("ThingSubclass", bound=Thing) + + +class ThingConnection(Generic[ThingSubclass], FieldTypedBaseDescriptor[ThingSubclass]): + """A descriptor that returns an instance of a Thing from this server. + + Thing connections allow `.Thing` instances to access other `.Thing` instances + in the same LabThings server. + """ + + def __init__(self, *, default_path: str | None = None) -> None: + """Declare a ThingConnection. + + :param default_name: The name of the Thing that will be connected by default. + """ + self._default_path = default_path + self._things: WeakKeyDictionary[Thing, ref[ThingSubclass]] = WeakKeyDictionary() + + def connect_thing(self, host_thing: Thing, connected_thing: ThingSubclass) -> None: + r"""Connect a `.Thing` to a `.ThingConnection`\ . + + This method sets up a ThingConnection on ``host_thing`` such that it will + supply ``connected_thing`` when accessed. + """ + if not isinstance(connected_thing, self.value_type): + msg = ( + f"Can't connect {connected_thing} to {host_thing}.{self.name}. " + f"This ThingConnection must be of type {self.value_type}." + ) + raise TypeError(msg) + self._things[host_thing] = ref(connected_thing) + + def instance_get(self, obj: Thing) -> ThingSubclass: + r"""Supply the connected `.Thing`\ . + + :raises ThingNotConnectedError: if the ThingConnection has not yet been set up. + """ + try: + thing_ref = self._things[obj] + return thing_ref() + except KeyError as e: + msg = f"{self.name} has not been connected to a Thing yet." + raise ThingNotConnectedError(msg) from e + # Note that ReferenceError is deliberately not handled: the Thing + # referred to by thing_ref should exist until the server has shut down. + + def __set__(self, obj: Thing, value: ThingSubclass) -> None: + """Raise an error as this is a read-only descriptor. + + :param obj: the `.Thing` on which the descriptor is defined. + :param value: the value being assigned. + + :raises AttributeError: this descriptor is not writeable. + """ + raise AttributeError("This descriptor is read-only.") From 5e333f658af5de9b65c3bf5eb0c0e24c2eedb9e9 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 15 Sep 2025 14:34:42 +0100 Subject: [PATCH 03/29] Completed implementation of thing_connection This commit adds the minimal set of features needed to use thing_connection in Thing code: * defines a function `thing_connection()` to make the class (primarily so we can type it as `Any` to avoid type clashes with field-style type annotation - see the related discussion for `property` in the typing notes section of the docstring). * adds a function to make thing connections in the server. * expose `thing_connection` in the API. * add a `name` property to `Thing` for convenience. --- src/labthings_fastapi/__init__.py | 2 + src/labthings_fastapi/exceptions.py | 9 +++ src/labthings_fastapi/server/__init__.py | 33 +++++++++ src/labthings_fastapi/thing.py | 5 ++ ...ing_connection.py => thing_connections.py} | 70 ++++++++++++++++--- 5 files changed, 108 insertions(+), 11 deletions(-) rename src/labthings_fastapi/{thing_connection.py => thing_connections.py} (59%) diff --git a/src/labthings_fastapi/__init__.py b/src/labthings_fastapi/__init__.py index a7bbabe..e68e850 100644 --- a/src/labthings_fastapi/__init__.py +++ b/src/labthings_fastapi/__init__.py @@ -20,6 +20,7 @@ """ from .thing import Thing +from .thing_connections import thing_connection from .thing_server_interface import ThingServerInterface from .properties import property, setting, DataProperty, DataSetting from .decorators import ( @@ -46,6 +47,7 @@ "DataProperty", "DataSetting", "thing_action", + "thing_connection", "fastapi_endpoint", "deps", "outputs", diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py index 40d16bd..509b5fe 100644 --- a/src/labthings_fastapi/exceptions.py +++ b/src/labthings_fastapi/exceptions.py @@ -82,3 +82,12 @@ class ThingNotConnectedError(RuntimeError): should be mocked), or because it has been accessed before ``__enter__`` has been called. """ + + +class ThingConnectionError(RuntimeError): + """A ThingConnection could not be set up. + + This error is raised if the LabThings server is unable to set up a + ThingConnection, for example because the named Thing does not exist, + or is of the wrong type, or is not specified and there is no default. + """ diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 8efc396..70d0e6c 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -18,6 +18,10 @@ from collections.abc import Mapping from types import MappingProxyType +from ..exceptions import ThingConnectionError +from ..thing_connections import ThingConnection +from ..utilities import class_attributes + from ..utilities.object_reference_to_object import ( object_reference_to_object, ) @@ -80,6 +84,7 @@ def __init__(self, settings_folder: Optional[str] = None) -> None: self.blob_data_manager.attach_to_app(self.app) self.add_things_view_to_app() self._things: dict[str, Thing] = {} + self.thing_connections: Mapping[str, Mapping[str, str | Sequence[str]]] = {} self.blocking_portal: Optional[BlockingPortal] = None self.startup_status: dict[str, str | dict] = {"things": {}} global _thing_servers # noqa: F824 @@ -225,6 +230,29 @@ def path_for_thing(self, name: str) -> str: raise KeyError(f"No thing named {name} has been added to this server.") return f"/{name}/" + def connect_things(self) -> None: + """Connect the `thing_connection` attributes of Things. + + A `.Thing` may have attributes defined as ``lt.thing_connection()``, which + """ + for thing_name, thing in self.things.items(): + config = self.thing_connections.get(thing_name, {}) + for attr_name, attr in class_attributes(thing): + if not isinstance(attr, ThingConnection): + continue + target = config.get(attr_name, attr.default) + if target is None: + raise ThingConnectionError( + f"{thing_name}.{attr_name} has not been configured " + "and has no default." + ) + if target not in self.things: + raise ThingConnectionError( + f"{thing_name}.{attr_name} is configured to connect to " + f"{target}, which does not exist." + ) + attr.connect_thing(thing, self.things[target]) + @asynccontextmanager async def lifespan(self, app: FastAPI) -> AsyncGenerator[None]: """Manage set up and tear down of the server and Things. @@ -249,6 +277,11 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None]: # We create a blocking portal to allow threaded code to call async code # in the event loop. self.blocking_portal = portal + + # Now we need to connect any ThingConnections. This is done here so that + # all of the Things are already created and added to the server. + self.connect_things() + # we __aenter__ and __aexit__ each Thing, which will in turn call the # synchronous __enter__ and __exit__ methods if they exist, to initialise # and shut down the hardware. NB we must make sure the blocking portal diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index fd49fcd..ccbd87f 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -101,6 +101,11 @@ def path(self) -> str: """The path at which the `.Thing` is exposed over HTTP.""" return self._thing_server_interface.path + @property + def name(self) -> str: + """The name of this Thing, as known to the server.""" + return self._thing_server_interface.name + async def __aenter__(self) -> Self: """Context management is used to set up/close the thing. diff --git a/src/labthings_fastapi/thing_connection.py b/src/labthings_fastapi/thing_connections.py similarity index 59% rename from src/labthings_fastapi/thing_connection.py rename to src/labthings_fastapi/thing_connections.py index 319e2df..fe73cb0 100644 --- a/src/labthings_fastapi/thing_connection.py +++ b/src/labthings_fastapi/thing_connections.py @@ -41,16 +41,16 @@ def say_hello(self) -> str: return self.thing_a.say_hello() """ -from typing import Generic, TypeVar, TYPE_CHECKING +from typing import Any, Generic, TypeVar, TYPE_CHECKING from weakref import WeakKeyDictionary, ref from .base_descriptor import FieldTypedBaseDescriptor -from .exceptions import ThingNotConnectedError +from .exceptions import ThingNotConnectedError, ThingConnectionError if TYPE_CHECKING: from .thing import Thing -ThingSubclass = TypeVar("ThingSubclass", bound=Thing) +ThingSubclass = TypeVar("ThingSubclass", bound="Thing") class ThingConnection(Generic[ThingSubclass], FieldTypedBaseDescriptor[ThingSubclass]): @@ -60,15 +60,25 @@ class ThingConnection(Generic[ThingSubclass], FieldTypedBaseDescriptor[ThingSubc in the same LabThings server. """ - def __init__(self, *, default_path: str | None = None) -> None: + def __init__(self, *, default: str | None = None) -> None: """Declare a ThingConnection. :param default_name: The name of the Thing that will be connected by default. """ - self._default_path = default_path - self._things: WeakKeyDictionary[Thing, ref[ThingSubclass]] = WeakKeyDictionary() - - def connect_thing(self, host_thing: Thing, connected_thing: ThingSubclass) -> None: + super().__init__() + self._default = default + self._things: WeakKeyDictionary["Thing", ref[ThingSubclass]] = ( + WeakKeyDictionary() + ) + + @property + def default(self) -> str | None: + """The name of the Thing that will be connected by default, if any.""" + return self._default + + def connect_thing( + self, host_thing: "Thing", connected_thing: ThingSubclass + ) -> None: r"""Connect a `.Thing` to a `.ThingConnection`\ . This method sets up a ThingConnection on ``host_thing`` such that it will @@ -79,10 +89,10 @@ def connect_thing(self, host_thing: Thing, connected_thing: ThingSubclass) -> No f"Can't connect {connected_thing} to {host_thing}.{self.name}. " f"This ThingConnection must be of type {self.value_type}." ) - raise TypeError(msg) + raise ThingConnectionError(msg) self._things[host_thing] = ref(connected_thing) - def instance_get(self, obj: Thing) -> ThingSubclass: + def instance_get(self, obj: "Thing") -> ThingSubclass: r"""Supply the connected `.Thing`\ . :raises ThingNotConnectedError: if the ThingConnection has not yet been set up. @@ -96,7 +106,7 @@ def instance_get(self, obj: Thing) -> ThingSubclass: # Note that ReferenceError is deliberately not handled: the Thing # referred to by thing_ref should exist until the server has shut down. - def __set__(self, obj: Thing, value: ThingSubclass) -> None: + def __set__(self, obj: "Thing", value: ThingSubclass) -> None: """Raise an error as this is a read-only descriptor. :param obj: the `.Thing` on which the descriptor is defined. @@ -105,3 +115,41 @@ def __set__(self, obj: Thing, value: ThingSubclass) -> None: :raises AttributeError: this descriptor is not writeable. """ raise AttributeError("This descriptor is read-only.") + + +def thing_connection(default: str | None = None) -> Any: + r"""Declare a connection to another `.Thing` in the same server. + + This function is a convenience wrapper around the `.ThingConnection` descriptor + class, and should be used in preference to using the descriptor directly. + The main reason to use the function is that it suppresses type errors when + using static type checkers such as `mypy` or `pyright`. + + In keeping with `.property` and `.setting`, the type of the attribute should + be the type of the connected `.Thing`\ . For example: + + .. code-block:: python + + import labthings_fastapi as lt + + + class ThingA(lt.Thing): ... + + + class ThingB(lt.Thing): + "A class that relies on ThingA." + + thing_a: ThingA = lt.thing_connection("thing_a") + + In the example above, using `.ThingConnection` directly would assign an object + with type ``ThingConnection[ThingA]`` to the attribute ``thing_a``, which is + typed as ``ThingA``\ . This would cause a type error. Using + `.thing_connection` suppresses this error, as its return type is `Any`\ . + + :param default: The name of the Thing that will be connected by default. + It is possible to omit this default or set it to ``None``\ , but if that + is done, it will cause an error unless the connection is explicitly made + in the server configuration. That is usually not desirable. + :return: A `.ThingConnection` instance. + """ + return ThingConnection(default=default) From 405ac090a82651ba6cca9833e36aed00afb578df Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 15 Sep 2025 14:36:23 +0100 Subject: [PATCH 04/29] Test code for thing_connection This checks that two things can be connected to each other. In writing this test, I realised my initial assumption that it would be easy to have circular dependencies and that these would not cause a problem wasn't true. See next commit for my solution to the circular dependency problem. The test covers the expected functionailty and anticipated errors in what I hope is a fairly bottom-up way. --- tests/test_thing_connection.py | 138 +++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 tests/test_thing_connection.py diff --git a/tests/test_thing_connection.py b/tests/test_thing_connection.py new file mode 100644 index 0000000..275b51e --- /dev/null +++ b/tests/test_thing_connection.py @@ -0,0 +1,138 @@ +"""Test the thing_connection module.""" + +import pytest +import labthings_fastapi as lt +from fastapi.testclient import TestClient + +from labthings_fastapi.exceptions import ThingConnectionError, ThingNotConnectedError + + +class ThingOne(lt.Thing): + """A class that will cause chaos if it can.""" + + thing_two: "ThingTwo" = lt.thing_connection("thing_two") + + @lt.thing_action + def say_hello(self) -> str: + """An example function.""" + return "Hello from thing_one." + + @lt.thing_action + def ask_other_thing(self) -> str: + """Ask ThingTwo to say hello.""" + return self.thing_two.say_hello() + + +class ThingTwo(lt.Thing): + """A class that relies on ThingOne.""" + + thing_one: ThingOne = lt.thing_connection("thing_one") + + @lt.thing_action + def say_hello(self) -> str: + """An example function.""" + return "Hello from thing_two." + + @lt.thing_action + def ask_other_thing(self) -> str: + """Ask ThingOne to say hello.""" + return self.thing_one.say_hello() + + +class ThingN(lt.Thing): + """A class that emulates ThingOne and ThingTwo more generically.""" + + other_thing: "ThingN" = lt.thing_connection() + + @lt.thing_action + def say_hello(self) -> str: + """An example function.""" + return f"Hello from {self.name}." + + @lt.thing_action + def ask_other_thing(self) -> str: + """Ask the other thing to say hello.""" + return self.other_thing.say_hello() + + +CONNECTIONS = { + "thing_one": {"other_thing": "thing_two"}, + "thing_two": {"other_thing": "thing_one"}, +} + + +@pytest.mark.parametrize( + ("cls_1", "cls_2", "connections"), + [ + (ThingOne, ThingTwo, None), + (ThingOne, ThingTwo, CONNECTIONS), + (ThingN, ThingN, CONNECTIONS), + ], +) +def test_thing_connection(cls_1, cls_2, connections) -> None: + """Check that two things can connect to each other. + + Note that this test includes a circular dependency, which is fine. + No checks are made for infinite loops: that's up to the author of the + Thing classes. Circular dependencies should not cause any problems for + the LabThings server. + """ + server = lt.ThingServer() + thing_one = server.add_thing("thing_one", cls_1) + thing_two = server.add_thing("thing_two", cls_2) + things = [thing_one, thing_two] + numbers = ["one", "two"] + if connections is not None: + server.thing_connections = connections + + # Check the things say hello correctly + for thing, num in zip(things, numbers, strict=True): + assert thing.say_hello() == f"Hello from thing_{num}." + + # Check the connections don't work initially, because they aren't connected + for thing in things: + with pytest.raises(ThingNotConnectedError): + thing.ask_other_thing() + + with TestClient(server.app) as client: + # The things should be connected as the server is now running + + # Check the things are connected, calling actions directly + for thing, num in zip(things, reversed(numbers), strict=True): + assert thing.ask_other_thing() == f"Hello from thing_{num}." + + # Check the same happens over "HTTP" (i.e. the TestClient) + for num, othernum in zip(numbers, reversed(numbers), strict=True): + thing = lt.ThingClient.from_url(f"/thing_{num}/", client=client) + assert thing.ask_other_thing() == f"Hello from thing_{othernum}." + assert thing.say_hello() == f"Hello from thing_{num}." + + +@pytest.mark.parametrize( + ("connections", "error"), + [ + # No default, no configuration - error should say so. + (None, "no default"), + # Configured to connect to a missing thing + ({"thing_one": {"other_thing": "non_existent_thing"}}, "does not exist"), + # Configured to connect to a thing that exists, but is the wrong type + ({"thing_one": {"other_thing": "thing_two"}}, "must be of type"), + ], +) +def test_thing_connection_errors(connections, error) -> None: + """Check that a ThingConnection without a default raises an error.""" + server = lt.ThingServer() + server.add_thing("thing_one", ThingN) + server.add_thing("thing_two", ThingTwo) + + if connections is not None: + server.thing_connections = connections + + with pytest.RaisesGroup(ThingConnectionError) as excinfo: + # Creating a TestClient should activate the connections + with TestClient(server.app): + pass + # excinfo contains an ExceptionGroup because TestClient runs in a + # task group, hence the use of RaisesGroup and the `.exceptions[0]` + # below. + assert error in str(excinfo.value.exceptions[0]) From e688dd72fc15db819401bd4397af99a7f05b6745 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 15 Sep 2025 14:43:28 +0100 Subject: [PATCH 05/29] Improve handling of field-typed descriptors. The previous implementation of dataclass-style field typing (i.e. annotating class attributes like `foo: int = lt.property()`) used `typing.get_type_hints()`. This was clean and readable but wasn't very efficient (it retrieves every type hint every time) and un-string-ized the annotations immediately. I have switched to an implementation that is lower-level (using `inspect.get_annotations` and manually traversing the MRO) which has two key benefits: * We only evaluate the one type hint we care about each time. * The type hint is lazily evaluated as it's needed. The latter is important: it means that annotations given as strings or modules using `from __future__ import annotations` will work as expected. In implementing and type checking this I realised that it's far simpler to have both types of `Property` inherit all the typing behaviour (rather than trying to have `FunctionalProperty` inherit only part of it). This changes a few things, none of which I believe matter in code using the library: * Field-style type hints are now checked for functional properties. Previously, they were just ignored. Now, we'll check them and raise an error if a type is specified that disagrees with the supplied function This means errors will appear in some places where they'd have been ignored, but the behaviour for valid code is unchanged. * Types must always wait for the descriptor to be added to a class before they work. This mostly means tests that used bare `FunctionalProperty` instances had to be updated to put these in the context of a class (any class will do).. --- src/labthings_fastapi/base_descriptor.py | 66 ++++++++++++++++++++++-- src/labthings_fastapi/properties.py | 23 ++------- tests/test_property.py | 12 ++++- 3 files changed, 74 insertions(+), 27 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index a0c0723..37dc9e4 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -14,7 +14,7 @@ from typing import overload, Generic, Mapping, TypeVar, TYPE_CHECKING from types import MappingProxyType import typing -from weakref import WeakKeyDictionary +from weakref import WeakKeyDictionary, ref, ReferenceType from typing_extensions import Self from .utilities.introspection import get_docstring, get_summary @@ -362,7 +362,9 @@ class FieldTypedBaseDescriptor(Generic[Value], BaseDescriptor[Value]): def __init__(self) -> None: """Initialise the FieldTypedBaseDescriptor.""" - self._type: type | None = None # Will be set in __set_name__ + super().__init__() + self._type: type | str | None = None # Will be set in __set_name__ + self._type_owner: ReferenceType[type] | None = None # For forward-references def __set_name__(self, owner: type[Thing], name: str) -> None: """Take note of the name and type. @@ -408,8 +410,7 @@ class MyThing(Thing): self._type = typing.get_args(self.__orig_class__)[0] # Check for annotations on the parent class - annotations = typing.get_type_hints(owner, include_extras=True) - field_annotation = annotations.get(name, None) + field_annotation, cls = get_class_attribute_annotation(owner, name) if field_annotation is not None: # We have been assigned to an annotated class attribute, e.g. # myprop: int = BaseProperty(0) @@ -420,6 +421,7 @@ class MyThing(Thing): f"with the inferred type of {self._type}." ) self._type = field_annotation + self._type_owner = ref(cls) if self._type is None: raise MissingTypeError( f"No type hint was found for property {name} on {owner}." @@ -433,12 +435,42 @@ def value_type(self) -> type[Value]: at the end of the class definition. If it is called too early, a `.DescriptorNotAddedToClassError` will be raised. + Accessing this property will attempt to resolve forward references, + i.e. type annotations that are strings. If there is an error resolving + the forward reference, a `.MissingTypeError` will be raised. + :return: the type of the descriptor's value. - :raises MissingTypeError: if the type is None or not specified. + :raises MissingTypeError: if the type is None, not resolvable, or not specified. """ self.assert_set_name_called() if self._type is None: raise MissingTypeError(f"No type hint was found for property {self.name}. ") + if isinstance(self._type, str): + # We have a forward reference, so we need to resolve it. + if self._type_owner is None: + raise MissingTypeError( + f"Can't resolve forward reference for type of {self.name} because " + "the class on which it was defined wasn't saved. This is a " + "LabThings bug - please report it." + ) + owner = self._type_owner() + if owner is None: + raise MissingTypeError( + f"Can't resolve forward reference for type of {self.name} because " + "the class on which it was defined has been garbage collected." + ) + try: + # We resolve the forward reference by evaluating it in the context of + # the module and class in which it was defined. + # Ruff flags it as insecure, but the string we are resolving is part of + # the source code: it is not user input. + self._type = eval( # noqa: S307 + self._type, vars(inspect.getmodule(owner)), vars(owner) + ) + except Exception as e: + raise MissingTypeError( + f"Can't resolve forward reference for type of {self.name}." + ) from e return self._type @@ -454,6 +486,30 @@ def value_type(self) -> type[Value]: ) +def get_class_attribute_annotation( + cls: type, name: str +) -> tuple[type | str, type] | tuple[None, None]: + """Retrieve the type annotation for an attribute of a class. + + This function retrieves the type annotation for an attribute of a class, + if one exists. It uses `inspect.get_annotations` to retrieve the + annotation without resolving forward references, and it searches + the class and its parents by traversing the method resolution order. + + :param cls: The class to inspect. + :param name: The name of the attribute whose annotation is required. + :return: A tuple of the annotation and the class on which it's defined (which + may be a parent of the class supplied), or ``(None, None)`` if there is + no annotation. A tuple is supplied so unpacking of the return value + will always work. + """ + for base in inspect.getmro(cls): + annotations = inspect.get_annotations(base) + if name in annotations: + return annotations[name], base + return (None, None) + + def get_class_attribute_docstrings(cls: type) -> Mapping[str, str]: """Retrieve docstrings for the attributes of a class. diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 104ed46..b5d45ee 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -72,7 +72,7 @@ class attribute. Documentation is in strings immediately following the ) from .utilities import labthings_data, wrap_plain_types_in_rootmodel from .utilities.introspection import return_type -from .base_descriptor import BaseDescriptor, FieldTypedBaseDescriptor +from .base_descriptor import FieldTypedBaseDescriptor from .exceptions import ( NotConnectedToServerError, ReadOnlyPropertyError, @@ -302,7 +302,7 @@ def property( ) -class BaseProperty(BaseDescriptor[Value], Generic[Value]): +class BaseProperty(FieldTypedBaseDescriptor[Value], Generic[Value]): """A descriptor that marks Properties on Things. This class is used to determine whether an attribute of a `.Thing` should @@ -319,15 +319,6 @@ def __init__(self) -> None: self._model: type[BaseModel] | None = None self.readonly: bool = False - @builtins.property - def value_type(self) -> type[Value]: - """The type of this descriptor's value. - - :raises NotImplementedError: because this method must be overridden. - :return: the type of the descriptor's value. - """ - raise NotImplementedError - @builtins.property def model(self) -> type[BaseModel]: """A Pydantic model for the property's type. @@ -451,9 +442,7 @@ def __set__(self, obj: Thing, value: Any) -> None: ) -class DataProperty( - FieldTypedBaseDescriptor[Value], BaseProperty[Value], Generic[Value] -): +class DataProperty(BaseProperty[Value], Generic[Value]): """A Property descriptor that acts like a regular variable. `.DataProperty` descriptors remember their value, and can be read and @@ -513,12 +502,6 @@ def __init__( ) self.readonly = readonly - @builtins.property - def value_type(self) -> type[Value]: # noqa: DOC201 - """The type of the descriptor's value.""" - self.assert_set_name_called() - return super().value_type - def instance_get(self, obj: Thing) -> Value: """Return the property's value. diff --git a/tests/test_property.py b/tests/test_property.py index 96ceef6..8f966d8 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -154,7 +154,11 @@ def test_baseproperty_type_and_model(): This checks baseproperty correctly wraps plain types in a `pydantic.RootModel`. """ - prop = tp.BaseProperty() + + class Example: + prop = tp.BaseProperty() + + prop = Example.prop # By default, we have no type so `.type` errors. with pytest.raises(tp.MissingTypeError): @@ -175,7 +179,11 @@ def test_baseproperty_type_and_model_pydantic(): This checks baseproperty behaves correctly when its type is a BaseModel instance. """ - prop = tp.BaseProperty() + + class Example: + prop = tp.BaseProperty() + + prop = Example.prop class MyModel(pydantic.BaseModel): foo: str From 28b2709c3560c5f8fb8d3bcb762099fca361a7ac Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 15 Sep 2025 21:53:41 +0100 Subject: [PATCH 06/29] Docstring fixes --- src/labthings_fastapi/properties.py | 2 ++ src/labthings_fastapi/server/__init__.py | 10 ++++++++++ src/labthings_fastapi/thing_connections.py | 13 ++++++++++++- 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index b5d45ee..1e31d38 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -601,6 +601,8 @@ def __init__( tools understand that it functions like a property. :param fget: the getter function, called when the property is read. + + :raises MissingTypeError: if the getter does not have a return type annotation. """ super().__init__() self._fget: ValueGetter = fget diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 70d0e6c..3aa3066 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -234,6 +234,16 @@ def connect_things(self) -> None: """Connect the `thing_connection` attributes of Things. A `.Thing` may have attributes defined as ``lt.thing_connection()``, which + will be populated after all `.Thing` instances are loaded on the server. + + This function is responsible for supplying the `.Thing` instances required + for each connection. This will be done by using the name specified either + in the connection's default, or in the configuration of the server. + + :raises ThingConnectionError: if a `.Thing` named in either the default + or the server configuration is missing or of the wrong type. If no + `.Thing` is specified (i.e. there is no default and it is not configured), + an error will also be raised. """ for thing_name, thing in self.things.items(): config = self.thing_connections.get(thing_name, {}) diff --git a/src/labthings_fastapi/thing_connections.py b/src/labthings_fastapi/thing_connections.py index fe73cb0..0f214d7 100644 --- a/src/labthings_fastapi/thing_connections.py +++ b/src/labthings_fastapi/thing_connections.py @@ -63,7 +63,7 @@ class ThingConnection(Generic[ThingSubclass], FieldTypedBaseDescriptor[ThingSubc def __init__(self, *, default: str | None = None) -> None: """Declare a ThingConnection. - :param default_name: The name of the Thing that will be connected by default. + :param default: The name of the Thing that will be connected by default. """ super().__init__() self._default = default @@ -83,6 +83,13 @@ def connect_thing( This method sets up a ThingConnection on ``host_thing`` such that it will supply ``connected_thing`` when accessed. + + :param host_thing: the `.Thing` on which the connection is defined. + :param connected_thing: the `.Thing` that will be available as the value + of the `.ThingConnection`\ . + + :raises ThingConnectionError: if the supplied `.Thing` is of the wrong + type. """ if not isinstance(connected_thing, self.value_type): msg = ( @@ -95,6 +102,10 @@ def connect_thing( def instance_get(self, obj: "Thing") -> ThingSubclass: r"""Supply the connected `.Thing`\ . + :param obj: The `.Thing` on which the connection is defined. + + :return: the `.Thing` instance that is connected. + :raises ThingNotConnectedError: if the ThingConnection has not yet been set up. """ try: From f747fc68eede0db164abc2042cd59163858ba8e3 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 24 Sep 2025 15:33:58 +0100 Subject: [PATCH 07/29] Revert to using `typing.get_type_hints` but evaluate lazily. `typing.get_type_hints` is the recommended way of getting the type hints for a class. Using this during `__set_name__` immediately evaluates any string annotations, i.e. it makes it impossible to do forward references. I'd previously rolled my own, using `__annotations__` and `eval` but this flagged security warnings with the linter, and misses some subtleties that are present in `get_type_hints`. While most of those subtleties aren't needed, I am prioritising code clarity over performance: instead of lazily evaluating the string type hint, I am just calling `get_type_hints` at a later point. This means there's one less bit of my code to go wrong. --- src/labthings_fastapi/base_descriptor.py | 91 ++++++++++++++++++------ 1 file changed, 69 insertions(+), 22 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 37dc9e4..3acca1f 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -361,13 +361,26 @@ class FieldTypedBaseDescriptor(Generic[Value], BaseDescriptor[Value]): """A BaseDescriptor that determines its type like a dataclass field.""" def __init__(self) -> None: - """Initialise the FieldTypedBaseDescriptor.""" + """Initialise the FieldTypedBaseDescriptor. + + Very little happens at initialisation time: most of the type determination + happens in ``__set_name__`` and ``value_type`` so that type hints can + be lazily evaluated. + """ super().__init__() - self._type: type | str | None = None # Will be set in __set_name__ - self._type_owner: ReferenceType[type] | None = None # For forward-references + self._type: type | None = None # the type of the descriptor's value. + # It may be set during __set_name__ if a type is available, or the + # first time `self.value_type` is accessed. + self._unevaluated_type_hint: str | None = None # Set in `__set_name__` + # Type hints are not un-stringized in `__set_name__` but we remember them + # for later evaluation in `value_type`. + self._owner: ReferenceType[type] | None = None # For forward-reference types + # When we evaluate the type hints in `value_type` we need a reference to + # the object on which they are defined, to provide the context for the + # evaluation. def __set_name__(self, owner: type[Thing], name: str) -> None: - """Take note of the name and type. + r"""Take note of the name and type. This function is where we determine the type of the property. It may be specified in two ways: either by subscripting the descriptor @@ -391,6 +404,16 @@ class MyThing(Thing): See :ref:`descriptors` for links to the Python docs about when this function is called. + For subscripted types (i.e. the first form above), we use + `typing.get_args` to retrieve the value type. This will be evaluated + immediately, resolving any forward references. + + We use `typing.get_type_hints` to resolve type hints on the owning + class. This takes care of a lot of subtleties like un-stringifying + forward references. In order to support forward references, we only + check for the existence of a type hint during ``__set_name__`` and + will evaluate it fully during ``value_type``\ . + :param owner: the `.Thing` subclass to which we are being attached. :param name: the name to which we have been assigned. @@ -406,25 +429,36 @@ class MyThing(Thing): # We have been instantiated with a subscript, e.g. BaseProperty[int]. # # __orig_class__ is set on generic classes when they are instantiated - # with a subscripted type. + # with a subscripted type. It is not available during __init__, which + # is why we check for it here. self._type = typing.get_args(self.__orig_class__)[0] # Check for annotations on the parent class - field_annotation, cls = get_class_attribute_annotation(owner, name) + field_annotation = inspect.get_annotations(owner).get(name, None) if field_annotation is not None: # We have been assigned to an annotated class attribute, e.g. # myprop: int = BaseProperty(0) if self._type is not None and self._type != field_annotation: + # As a rule, if _type is already set, we don't expect any + # annotation on the attribute, so this error should not + # be a frequent occurrence. raise InconsistentTypeError( f"Property {name} on {owner} has conflicting types.\n\n" f"The field annotation of {field_annotation} conflicts " f"with the inferred type of {self._type}." ) - self._type = field_annotation - self._type_owner = ref(cls) - if self._type is None: + self._unevaluated_type_hint = field_annotation + self._owner = ref(owner) + + # Ensure a type is specified. + # If we've not set _type by now, we are not going to set it, and the + # descriptor will not work properly. It's beest to raise an error now. + # Note that we need to specify the attribute name, as the exception + # will appear to come from the end of the class definition, and not + # from the descriptor definition. + if self._type is None and self._unevaluated_type_hint is None: raise MissingTypeError( - f"No type hint was found for property {name} on {owner}." + f"No type hint was found for attribute {name} on {owner}." ) @builtins.property @@ -443,34 +477,47 @@ def value_type(self) -> type[Value]: :raises MissingTypeError: if the type is None, not resolvable, or not specified. """ self.assert_set_name_called() - if self._type is None: - raise MissingTypeError(f"No type hint was found for property {self.name}. ") - if isinstance(self._type, str): + if self._unevaluated_type_hint is not None: # We have a forward reference, so we need to resolve it. - if self._type_owner is None: + if self._owner is None: raise MissingTypeError( f"Can't resolve forward reference for type of {self.name} because " "the class on which it was defined wasn't saved. This is a " "LabThings bug - please report it." ) - owner = self._type_owner() + owner = self._owner() if owner is None: raise MissingTypeError( f"Can't resolve forward reference for type of {self.name} because " "the class on which it was defined has been garbage collected." ) try: - # We resolve the forward reference by evaluating it in the context of - # the module and class in which it was defined. - # Ruff flags it as insecure, but the string we are resolving is part of - # the source code: it is not user input. - self._type = eval( # noqa: S307 - self._type, vars(inspect.getmodule(owner)), vars(owner) - ) + # Resolving a forward reference has quirks, and rather than tie us + # to undocumented implementation details of `typing` we just use + # `typing.get_type_hints`. + # This isn't efficient (it resolves everything, rather than just + # the one annotation we need, and it traverses the MRO when we know + # the class we're defined on) but it is part of the public API, + # and therefore much less likely to break. + # + # Note that we already checked there was an annotation in + # __set_name__. + hints = typing.get_type_hints(owner, include_extras=True) + self._type = hints[self.name] except Exception as e: raise MissingTypeError( f"Can't resolve forward reference for type of {self.name}." ) from e + if self._type is None: + # We should never reach this line: if `__set_name__` was called, we'd + # have raised an exception there if _type was None. If `__set_name__` + # has not been called, `self.assert_set_name_called()` would have failed. + # This block is required for `mypy` to know that self._type is not None. + raise MissingTypeError( + f"No type hint was found for property {self.name}. This may indicate " + "a bug in LabThings, as the error should have been caught before now." + ) + return self._type From 9816338506143d6938719e9fa649cf2a9a29aaaf Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 24 Sep 2025 15:35:05 +0100 Subject: [PATCH 08/29] Support multiple things in a connection It's now possible to specify the type as a Mapping or Union, in order to either permit multiple Things, or allow None. --- src/labthings_fastapi/thing_connections.py | 192 ++++++++++++++++----- 1 file changed, 147 insertions(+), 45 deletions(-) diff --git a/src/labthings_fastapi/thing_connections.py b/src/labthings_fastapi/thing_connections.py index 0f214d7..6376334 100644 --- a/src/labthings_fastapi/thing_connections.py +++ b/src/labthings_fastapi/thing_connections.py @@ -41,8 +41,11 @@ def say_hello(self) -> str: return self.thing_a.say_hello() """ +import types from typing import Any, Generic, TypeVar, TYPE_CHECKING -from weakref import WeakKeyDictionary, ref +from collections.abc import Mapping, Sequence +import typing +from weakref import ReferenceType, WeakKeyDictionary, ref, WeakValueDictionary from .base_descriptor import FieldTypedBaseDescriptor from .exceptions import ThingNotConnectedError, ThingConnectionError @@ -51,84 +54,183 @@ def say_hello(self) -> str: ThingSubclass = TypeVar("ThingSubclass", bound="Thing") +ConnectedThings = TypeVar( + "ConnectedThings", + bound="Mapping[str, Thing] | Thing | None", +) -class ThingConnection(Generic[ThingSubclass], FieldTypedBaseDescriptor[ThingSubclass]): - """A descriptor that returns an instance of a Thing from this server. +class ThingConnection( + Generic[ConnectedThings], FieldTypedBaseDescriptor[ConnectedThings] +): + r"""Descriptor that returns other Things from the server. - Thing connections allow `.Thing` instances to access other `.Thing` instances - in the same LabThings server. + A `.ThingConnection` provides either one or several + `.Thing` instances as a property of a `.Thing`\ . This allows `.Thing`\ s + to communicate with each other within the server, including accessing + attributes that are not exposed over HTTP. + + While it is possible to dynamically retrieve a `.Thing` from the `.ThingServer` + this is not recommended: using Thing Connections ensures all the `.Thing` + instances are available before the server starts, reducing the likelihood + of run-time crashes. + + The usual way of creating these connections is the function + `.thing_connection`\ . This class and its subclasses are not usually + instantiated directly. + + The type of the `.ThingConnection` attribute is key to its operation. + It should be assigned to an attribute typed either as a `.Thing` subclass, + a mapping of strings to `.Thing` or subclass instances, or an optional + `.Thing` instance: + + .. code-block:: python + + class OtherExample(lt.Thing): + pass + + + class Example(lt.Thing): + # This will always evaluate to an `OtherExample` + other_thing: OtherExample = lt.thing_connection("other_thing") + + # This may evaluate to an `OtherExample` or `None` + optional: OtherExample | None = lt.thing_connection("other_thing") + + # This evaluates to a mapping of `str` to `.Thing` instances + things: Mapping[str, OtherExample] = lt.thing_connection(["thing_a"]) """ - def __init__(self, *, default: str | None = None) -> None: + def __init__(self, *, default: str | None | Sequence[str] = None) -> None: """Declare a ThingConnection. - :param default: The name of the Thing that will be connected by default. + :param default: The name of the Thing(s) that will be connected by default. + + If the type is optional (e.g. ``ThingSubclass | None``) a default + value of ``None`` will result in the connection evaluating to ``None`` + unless it has been configured by the server. + + If the type is not optional, a default value of ``None`` will result + in an error, unless the server has set another value in its + configuration. + + If the type is a mapping of `str` to `.Thing` the default should be + of type `Sequence[str]` (and could be an empty list). """ super().__init__() self._default = default - self._things: WeakKeyDictionary["Thing", ref[ThingSubclass]] = ( - WeakKeyDictionary() - ) + self._things: WeakKeyDictionary[ + "Thing", ReferenceType["Thing"] | WeakValueDictionary[str, "Thing"] | None + ] = WeakKeyDictionary() + + @property + def thing_type(self) -> tuple[type["Thing"], ...]: + r"""The `.Thing` subclass(es) returned by this connection. + + A tuple is returned to allow for optional thing conections that + are typed as the union of two Thing types. It will work with + `isinstance`\ . + """ + if not self.is_mapping: + return self.value_type + # is_mapping already checks the type is a `Mapping`, so + # we can just look at its arguments. + _, thing_type = typing.get_args(self.value_type) + return thing_type + + @property + def is_mapping(self) -> bool: + """Whether we return a mapping of strings to Things, or a single Thing.""" + return typing.get_origin(self.value_type) is Mapping + + @property + def is_optional(self) -> bool: + """Whether ``None`` or an empty mapping is an allowed value.""" + if typing.get_origin(self.value_type) in (types.UnionType, typing.Union): + if types.NoneType in typing.get_args(self.value_type): + return True + return False @property - def default(self) -> str | None: + def default(self) -> str | Sequence[str] | None: """The name of the Thing that will be connected by default, if any.""" return self._default - def connect_thing( - self, host_thing: "Thing", connected_thing: ThingSubclass - ) -> None: - r"""Connect a `.Thing` to a `.ThingConnection`\ . + def __set__(self, obj: "Thing", value: ThingSubclass) -> None: + """Raise an error as this is a read-only descriptor. + + :param obj: the `.Thing` on which the descriptor is defined. + :param value: the value being assigned. + + :raises AttributeError: this descriptor is not writeable. + """ + raise AttributeError("This descriptor is read-only.") + + def connect(self, host: "Thing", target: ConnectedThings) -> None: + r"""Connect a `.Thing` (or several) to a `.ThingConnection`\ . This method sets up a ThingConnection on ``host_thing`` such that it will - supply ``connected_thing`` when accessed. + supply ``target`` when accessed. - :param host_thing: the `.Thing` on which the connection is defined. - :param connected_thing: the `.Thing` that will be available as the value - of the `.ThingConnection`\ . + :param host: the `.Thing` on which the connection is defined. + :param target: the `.Thing` that will be available as the value + of the `.ThingConnection` or a sequence of `.Thing` instances, + or `None`\ . :raises ThingConnectionError: if the supplied `.Thing` is of the wrong - type. + type, if a sequence is supplied when a single `.Thing` is required, + or if `None` is supplied and the connection is not optional. """ - if not isinstance(connected_thing, self.value_type): - msg = ( - f"Can't connect {connected_thing} to {host_thing}.{self.name}. " - f"This ThingConnection must be of type {self.value_type}." - ) - raise ThingConnectionError(msg) - self._things[host_thing] = ref(connected_thing) - - def instance_get(self, obj: "Thing") -> ThingSubclass: - r"""Supply the connected `.Thing`\ . + base_msg = f"Can't connect %s to {host.name}.{self.name} " + thing_type_msg = f"{base_msg} because it is not of type {self.thing_type}." + unexpected_sequence_msg = f"{base_msg} because a single Thing is needed." + expected_sequence_msg = f"{base_msg} because a sequence of Things is needed." + not_optional_msg = f"{base_msg} because it is not optional and has no default." + if target is None: + if self.is_optional: + self._things[host] = None + else: + raise ThingConnectionError(not_optional_msg) + elif isinstance(target, Sequence): + if self.is_mapping: + for t in target: + if not isinstance(t, self.thing_type): + raise ThingConnectionError(thing_type_msg % repr(t)) + self._things[host] = WeakValueDictionary({t.name: t for t in target}) + else: + raise ThingConnectionError(unexpected_sequence_msg % target) + else: + if not self.is_mapping: + if not isinstance(target, self.thing_type): + raise ThingConnectionError(thing_type_msg % repr(target)) + self._things[host] = ref(target) + else: + raise ThingConnectionError(expected_sequence_msg % target) + + def instance_get(self, obj: "Thing") -> ConnectedThings: + r"""Supply the connected `.Thing`\ (s). :param obj: The `.Thing` on which the connection is defined. - :return: the `.Thing` instance that is connected. + :return: the `.Thing` instance(s) connected. :raises ThingNotConnectedError: if the ThingConnection has not yet been set up. """ + msg = f"{self.name} has not been connected to a Thing yet." try: - thing_ref = self._things[obj] - return thing_ref() + val = self._things[obj] except KeyError as e: - msg = f"{self.name} has not been connected to a Thing yet." raise ThingNotConnectedError(msg) from e # Note that ReferenceError is deliberately not handled: the Thing # referred to by thing_ref should exist until the server has shut down. - - def __set__(self, obj: "Thing", value: ThingSubclass) -> None: - """Raise an error as this is a read-only descriptor. - - :param obj: the `.Thing` on which the descriptor is defined. - :param value: the value being assigned. - - :raises AttributeError: this descriptor is not writeable. - """ - raise AttributeError("This descriptor is read-only.") + if isinstance(val, ReferenceType): + return val() + else: + # This works for None or for WeakValueDictionary() + return val -def thing_connection(default: str | None = None) -> Any: +def thing_connection(default: str | Sequence[str] | None = None) -> Any: r"""Declare a connection to another `.Thing` in the same server. This function is a convenience wrapper around the `.ThingConnection` descriptor From 8cb258ce5d9e1a1176ee04a616222ba177d99d57 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 24 Sep 2025 15:56:57 +0100 Subject: [PATCH 09/29] Move more error checking into `ThingConnection` I've kept `ThingServer.connect_things` as simple as possible, by moving error checks into `ThingConnection`. --- src/labthings_fastapi/server/__init__.py | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 3aa3066..6301c6e 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -7,7 +7,7 @@ """ from __future__ import annotations -from typing import Any, AsyncGenerator, Optional, Sequence, TypeVar +from typing import Any, AsyncGenerator, Optional, TypeVar import os.path import re @@ -15,7 +15,7 @@ from fastapi.middleware.cors import CORSMiddleware from anyio.from_thread import BlockingPortal from contextlib import asynccontextmanager, AsyncExitStack -from collections.abc import Mapping +from collections.abc import Mapping, Sequence from types import MappingProxyType from ..exceptions import ThingConnectionError @@ -251,17 +251,15 @@ def connect_things(self) -> None: if not isinstance(attr, ThingConnection): continue target = config.get(attr_name, attr.default) - if target is None: - raise ThingConnectionError( - f"{thing_name}.{attr_name} has not been configured " - "and has no default." - ) - if target not in self.things: - raise ThingConnectionError( - f"{thing_name}.{attr_name} is configured to connect to " - f"{target}, which does not exist." - ) - attr.connect_thing(thing, self.things[target]) + try: + if isinstance(target, str | None): + attr.connect(thing, self.things[target]) + elif isinstance(target, Sequence): + attr.connect(thing, [self.things[t] for t in target]) + except KeyError as e: + msg = f"Trying to connect {thing_name}.{attr_name} to {target}. " + msg += "Target Thing does not exist." + raise ThingConnectionError(msg) from e @asynccontextmanager async def lifespan(self, app: FastAPI) -> AsyncGenerator[None]: From 24998bff6cb3704d16acad0e575316774ebe2390 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 24 Sep 2025 21:22:40 +0100 Subject: [PATCH 10/29] Got tests passing Fixed a bug that did the wrong thing if a connection was configured to be None, and fixed the remaining test that was failing because of an error I'd not anticipated. --- src/labthings_fastapi/server/__init__.py | 4 +- tests/test_thing_connection.py | 53 +++++++++++++++++------- 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 6301c6e..78322c4 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -252,7 +252,9 @@ def connect_things(self) -> None: continue target = config.get(attr_name, attr.default) try: - if isinstance(target, str | None): + if target is None: + attr.connect(thing, None) + elif isinstance(target, str): attr.connect(thing, self.things[target]) elif isinstance(target, Sequence): attr.connect(thing, [self.things[t] for t in target]) diff --git a/tests/test_thing_connection.py b/tests/test_thing_connection.py index 275b51e..41455ff 100644 --- a/tests/test_thing_connection.py +++ b/tests/test_thing_connection.py @@ -1,5 +1,6 @@ """Test the thing_connection module.""" +from collections.abc import Mapping import pytest import labthings_fastapi as lt from fastapi.testclient import TestClient @@ -10,7 +11,10 @@ class ThingOne(lt.Thing): """A class that will cause chaos if it can.""" - thing_two: "ThingTwo" = lt.thing_connection("thing_two") + other_thing: "ThingTwo" = lt.thing_connection("thing_two") + multiple_things: Mapping[str, lt.Thing] = lt.thing_connection( + ["thing_one", "thing_two"] + ) @lt.thing_action def say_hello(self) -> str: @@ -20,13 +24,13 @@ def say_hello(self) -> str: @lt.thing_action def ask_other_thing(self) -> str: """Ask ThingTwo to say hello.""" - return self.thing_two.say_hello() + return self.other_thing.say_hello() class ThingTwo(lt.Thing): """A class that relies on ThingOne.""" - thing_one: ThingOne = lt.thing_connection("thing_one") + other_thing: ThingOne = lt.thing_connection("thing_one") @lt.thing_action def say_hello(self) -> str: @@ -36,7 +40,7 @@ def say_hello(self) -> str: @lt.thing_action def ask_other_thing(self) -> str: """Ask ThingOne to say hello.""" - return self.thing_one.say_hello() + return self.other_thing.say_hello() class ThingN(lt.Thing): @@ -55,6 +59,22 @@ def ask_other_thing(self) -> str: return self.other_thing.say_hello() +class ThingOfWrongType(lt.Thing): + """A Thing that has no other attributes.""" + + pass + + +def test_type_analysis(): + """Check the correct properties are inferred from the type hints.""" + assert ThingOne.other_thing.is_optional is False + assert ThingOne.other_thing.is_mapping is False + assert ThingOne.other_thing.thing_type is ThingTwo + assert ThingOne.multiple_things.is_optional is False + assert ThingOne.multiple_things.is_mapping is True + assert ThingOne.multiple_things.thing_type is lt.Thing + + CONNECTIONS = { "thing_one": {"other_thing": "thing_two"}, "thing_two": {"other_thing": "thing_one"}, @@ -111,22 +131,25 @@ def test_thing_connection(cls_1, cls_2, connections) -> None: @pytest.mark.parametrize( ("connections", "error"), [ - # No default, no configuration - error should say so. - (None, "no default"), - # Configured to connect to a missing thing + ({}, "no default"), ({"thing_one": {"other_thing": "non_existent_thing"}}, "does not exist"), - # Configured to connect to a thing that exists, but is the wrong type - ({"thing_one": {"other_thing": "thing_two"}}, "must be of type"), + ({"thing_one": {"other_thing": ("thing_one", "thing_one")}}, "single Thing"), + ({"thing_one": {"other_thing": "wrong_type_thing"}}, "not of type"), + ({"thing_one": {"other_thing": "thing_one"}}, None), ], ) -def test_thing_connection_errors(connections, error) -> None: - """Check that a ThingConnection without a default raises an error.""" +def test_connections_no_default(connections, error): + """Check a connection with no default and no config errors out.""" server = lt.ThingServer() - server.add_thing("thing_one", ThingN) - server.add_thing("thing_two", ThingTwo) + thing_one = server.add_thing("thing_one", ThingN) + server.add_thing("wrong_type_thing", ThingOfWrongType) - if connections is not None: - server.thing_connections = connections + server.thing_connections = connections + + if error is None: + with TestClient(server.app): + assert thing_one.other_thing is thing_one + return with pytest.RaisesGroup(ThingConnectionError) as excinfo: # Creating a TestClient should activate the connections From 869bd73340028563cf23d6cb0b045e2b54058d10 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Wed, 24 Sep 2025 21:30:38 +0100 Subject: [PATCH 11/29] Tidy up some imports --- src/labthings_fastapi/thing_connections.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/labthings_fastapi/thing_connections.py b/src/labthings_fastapi/thing_connections.py index 6376334..800557b 100644 --- a/src/labthings_fastapi/thing_connections.py +++ b/src/labthings_fastapi/thing_connections.py @@ -41,10 +41,9 @@ def say_hello(self) -> str: return self.thing_a.say_hello() """ -import types -from typing import Any, Generic, TypeVar, TYPE_CHECKING +from types import EllipsisType, NoneType, UnionType +from typing import Any, Generic, TypeVar, TYPE_CHECKING, Union, get_args, get_origin from collections.abc import Mapping, Sequence -import typing from weakref import ReferenceType, WeakKeyDictionary, ref, WeakValueDictionary from .base_descriptor import FieldTypedBaseDescriptor from .exceptions import ThingNotConnectedError, ThingConnectionError @@ -101,7 +100,9 @@ class Example(lt.Thing): things: Mapping[str, OtherExample] = lt.thing_connection(["thing_a"]) """ - def __init__(self, *, default: str | None | Sequence[str] = None) -> None: + def __init__( + self, *, default: str | None | Sequence[str] | EllipsisType = ... + ) -> None: """Declare a ThingConnection. :param default: The name of the Thing(s) that will be connected by default. @@ -135,19 +136,19 @@ def thing_type(self) -> tuple[type["Thing"], ...]: return self.value_type # is_mapping already checks the type is a `Mapping`, so # we can just look at its arguments. - _, thing_type = typing.get_args(self.value_type) + _, thing_type = get_args(self.value_type) return thing_type @property def is_mapping(self) -> bool: """Whether we return a mapping of strings to Things, or a single Thing.""" - return typing.get_origin(self.value_type) is Mapping + return get_origin(self.value_type) is Mapping @property def is_optional(self) -> bool: """Whether ``None`` or an empty mapping is an allowed value.""" - if typing.get_origin(self.value_type) in (types.UnionType, typing.Union): - if types.NoneType in typing.get_args(self.value_type): + if get_origin(self.value_type) in (UnionType, Union): + if NoneType in get_args(self.value_type): return True return False From 31350b44412f61828bdfb639312a480436f45c2e Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 25 Sep 2025 23:15:51 +0100 Subject: [PATCH 12/29] Testing and improvements I've covered 100% of `thing_connections.py` with unit tests, with bottom-up tests that don't use the server or `Thing`, as well as more realistic tests that do use `Thing` and `ThingServer`. This uncovered a few issues that I've fixed, in particular with `thing_type` and with error handling logic in `connect`. I've now split `connect` into `_pick_things` (which finds the Thing(s) to connect) and `connect` (which makes sure they match the type hint and supplies context for error messages). I think this is clearer. I think the split also makes it more testable, as I can test the logic in `_pick_things` separately. --- src/labthings_fastapi/server/__init__.py | 27 +- src/labthings_fastapi/thing_connections.py | 268 ++++++++++--- tests/test_thing_connection.py | 430 +++++++++++++++++---- 3 files changed, 581 insertions(+), 144 deletions(-) diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 78322c4..5dc7e22 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -18,7 +18,7 @@ from collections.abc import Mapping, Sequence from types import MappingProxyType -from ..exceptions import ThingConnectionError +from ..exceptions import ThingConnectionError as ThingConnectionError from ..thing_connections import ThingConnection from ..utilities import class_attributes @@ -230,7 +230,7 @@ def path_for_thing(self, name: str) -> str: raise KeyError(f"No thing named {name} has been added to this server.") return f"/{name}/" - def connect_things(self) -> None: + def _connect_things(self) -> None: """Connect the `thing_connection` attributes of Things. A `.Thing` may have attributes defined as ``lt.thing_connection()``, which @@ -240,28 +240,17 @@ def connect_things(self) -> None: for each connection. This will be done by using the name specified either in the connection's default, or in the configuration of the server. - :raises ThingConnectionError: if a `.Thing` named in either the default - or the server configuration is missing or of the wrong type. If no - `.Thing` is specified (i.e. there is no default and it is not configured), - an error will also be raised. + `.ThingConnectionError` will be raised by code called by this method if + the connection cannot be provided. See `.ThingConnection.connect` for more + details. """ for thing_name, thing in self.things.items(): config = self.thing_connections.get(thing_name, {}) for attr_name, attr in class_attributes(thing): if not isinstance(attr, ThingConnection): continue - target = config.get(attr_name, attr.default) - try: - if target is None: - attr.connect(thing, None) - elif isinstance(target, str): - attr.connect(thing, self.things[target]) - elif isinstance(target, Sequence): - attr.connect(thing, [self.things[t] for t in target]) - except KeyError as e: - msg = f"Trying to connect {thing_name}.{attr_name} to {target}. " - msg += "Target Thing does not exist." - raise ThingConnectionError(msg) from e + target = config.get(attr_name, ...) + attr.connect(thing, self.things, target) @asynccontextmanager async def lifespan(self, app: FastAPI) -> AsyncGenerator[None]: @@ -290,7 +279,7 @@ async def lifespan(self, app: FastAPI) -> AsyncGenerator[None]: # Now we need to connect any ThingConnections. This is done here so that # all of the Things are already created and added to the server. - self.connect_things() + self._connect_things() # we __aenter__ and __aexit__ each Thing, which will in turn call the # synchronous __enter__ and __exit__ methods if they exist, to initialise diff --git a/src/labthings_fastapi/thing_connections.py b/src/labthings_fastapi/thing_connections.py index 800557b..6acec8a 100644 --- a/src/labthings_fastapi/thing_connections.py +++ b/src/labthings_fastapi/thing_connections.py @@ -43,7 +43,7 @@ def say_hello(self) -> str: from types import EllipsisType, NoneType, UnionType from typing import Any, Generic, TypeVar, TYPE_CHECKING, Union, get_args, get_origin -from collections.abc import Mapping, Sequence +from collections.abc import Mapping, Iterable from weakref import ReferenceType, WeakKeyDictionary, ref, WeakValueDictionary from .base_descriptor import FieldTypedBaseDescriptor from .exceptions import ThingNotConnectedError, ThingConnectionError @@ -101,7 +101,7 @@ class Example(lt.Thing): """ def __init__( - self, *, default: str | None | Sequence[str] | EllipsisType = ... + self, *, default: str | None | Iterable[str] | EllipsisType = ... ) -> None: """Declare a ThingConnection. @@ -116,7 +116,7 @@ def __init__( configuration. If the type is a mapping of `str` to `.Thing` the default should be - of type `Sequence[str]` (and could be an empty list). + of type `Iterable[str]` (and could be an empty list). """ super().__init__() self._default = default @@ -128,16 +128,23 @@ def __init__( def thing_type(self) -> tuple[type["Thing"], ...]: r"""The `.Thing` subclass(es) returned by this connection. - A tuple is returned to allow for optional thing conections that + A tuple is returned to allow for optional thing connections that are typed as the union of two Thing types. It will work with `isinstance`\ . """ - if not self.is_mapping: - return self.value_type - # is_mapping already checks the type is a `Mapping`, so - # we can just look at its arguments. - _, thing_type = get_args(self.value_type) - return thing_type + thing_type = self.value_type + if self.is_mapping: + # is_mapping already checks the type is a `Mapping`, so + # we can just look at its arguments. + _, thing_type = get_args(self.value_type) + if get_origin(thing_type) in (UnionType, Union): + # If it's a Union, we may have an optional type, in which + # case we want to exclude None. + return tuple(t for t in get_args(thing_type) if t is not NoneType) + else: + # If it's not a Union, it should be a single Thing subclass + # so wrap it in a tuple. + return (thing_type,) @property def is_mapping(self) -> bool: @@ -153,7 +160,7 @@ def is_optional(self) -> bool: return False @property - def default(self) -> str | Sequence[str] | None: + def default(self) -> str | Iterable[str] | None | EllipsisType: """The name of the Thing that will be connected by default, if any.""" return self._default @@ -167,46 +174,136 @@ def __set__(self, obj: "Thing", value: ThingSubclass) -> None: """ raise AttributeError("This descriptor is read-only.") - def connect(self, host: "Thing", target: ConnectedThings) -> None: - r"""Connect a `.Thing` (or several) to a `.ThingConnection`\ . + def _pick_things( + self, + things: "Mapping[str, Thing]", + target: str | Iterable[str] | None | EllipsisType, + ) -> "Iterable[Thing]": + r"""Pick the Things we should connect to from a list. - This method sets up a ThingConnection on ``host_thing`` such that it will - supply ``target`` when accessed. + This function is used internally by `.ThingConnection.connect` to choose + the Things we return when the `.ThingConnection` is accessed. - :param host: the `.Thing` on which the connection is defined. - :param target: the `.Thing` that will be available as the value - of the `.ThingConnection` or a sequence of `.Thing` instances, - or `None`\ . + :param things: the available `.Thing` instances on the server. + :param target: the name(s) we should connect to, or `None` to set the + connection to `None` (if it is optional). A special value is `...` + which will pick the `.Thing` instannce(s) matching this connection's + type hint. :raises ThingConnectionError: if the supplied `.Thing` is of the wrong type, if a sequence is supplied when a single `.Thing` is required, or if `None` is supplied and the connection is not optional. + :raises TypeError: if ``target`` is not one of the allowed types. + + `KeyError` will also be raised if names specified in ``target`` do not + exist in ``things``\ . + + :return: a list of `.Thing` instances to supply in response to ``__get__``\ . """ - base_msg = f"Can't connect %s to {host.name}.{self.name} " - thing_type_msg = f"{base_msg} because it is not of type {self.thing_type}." - unexpected_sequence_msg = f"{base_msg} because a single Thing is needed." - expected_sequence_msg = f"{base_msg} because a sequence of Things is needed." - not_optional_msg = f"{base_msg} because it is not optional and has no default." if target is None: - if self.is_optional: - self._things[host] = None - else: - raise ThingConnectionError(not_optional_msg) - elif isinstance(target, Sequence): + return [] + elif target is ...: + return [ + thing + for _, thing in things.items() + if isinstance(thing, self.thing_type) + ] + elif isinstance(target, str): + if not isinstance(things[target], self.thing_type): + raise ThingConnectionError(f"{target} is the wrong type") + return [things[target]] + elif isinstance(target, Iterable): + for t in target: + if not isinstance(things[t], self.thing_type): + raise ThingConnectionError(f"{t} is the wrong type") + return [things[t] for t in target] + msg = "The target specified for a ThingConnection ({target}) has the wrong " + msg += "type. See ThingConnection.connect() docstring for details." + raise TypeError(msg) + + def connect( + self, + host: "Thing", + things: "Mapping[str, Thing]", + target: str | Iterable[str] | None | EllipsisType = ..., + ) -> None: + r"""Find the `.Thing`\ (s) we should supply when accessed. + + This method sets up a ThingConnection on ``host_thing`` by finding the + `.Thing` instance(s) it should supply when its ``__get__`` method is + called. The logic for determining this is: + + * If ``target`` is specified, we look for the specified `.Thing`\ (s). + ``None`` means we should return ``None`` - that's only allowed if the + type hint permits it. + * If ``target`` is not specified or is ``...`` we use the default value + set when the connection was defined. + * If the default value was ``...`` and no target was specified, we will + attempt to find the `.Thing` by type. Most of the time, this is the + desired behaviour. + + If the type of this connection is a ``Mapping``\ , ``target`` should be + a sequence of names. This sequence may be empty. + + ``None`` is treated as equivalent to the empty list, and a list with + one name in it is treated as equivalent to a single name. + + If the type hint of this connection does not permit ``None``\ , and + either ``None`` is specified, or no ``target`` is given and the default + is set as ``None``\ , then an error will be raised. ``None`` will only + be returned at runtime if it is permitted by the type hint. + + :param host: the `.Thing` on which the connection is defined. + :param things: the available `.Thing` instances on the server. + :param target: the name(s) we should connect to, or `None` to set the + connection to `None` (if it is optional). The default is `...` + which will use the default that was set when this `.ThingConnection` + was defined. + + :raises ThingConnectionError: if the supplied `.Thing` is of the wrong + type, if a sequence is supplied when a single `.Thing` is required, + or if `None` is supplied and the connection is not optional. + """ + used_target = self.default if target is ... else target + try: + # First, explicitly check for None so we can raise a helpful error. + if used_target is None and not self.is_optional and not self.is_mapping: + raise ThingConnectionError("it must be set in configuration") + # Most of the logic is split out into `_pick_things` to separate + # picking the Things from turning them into the correct mapping/reference. + picked = self._pick_things(things, used_target) if self.is_mapping: - for t in target: - if not isinstance(t, self.thing_type): - raise ThingConnectionError(thing_type_msg % repr(t)) - self._things[host] = WeakValueDictionary({t.name: t for t in target}) + # Mappings may have any number of entries, so no more validation needed. + self._things[host] = WeakValueDictionary({t.name: t for t in picked}) + elif len(picked) == 0: + if self.is_optional: + # Optional things may be set to None without an error. + self._things[host] = None + else: + # Otherwise a single Thing is required, so raise an error. + raise ThingConnectionError("no matching Thing was found") + elif len(picked) == 1: + # A single Thing is found: we can safely use this. + self._things[host] = ref(picked[0]) else: - raise ThingConnectionError(unexpected_sequence_msg % target) - else: - if not self.is_mapping: - if not isinstance(target, self.thing_type): - raise ThingConnectionError(thing_type_msg % repr(target)) - self._things[host] = ref(target) + # If more than one Thing is found (and we're not a mapping) this is + # an error. + raise ThingConnectionError("it can't connect to multiple Things") + except (ThingConnectionError, KeyError) as e: + reason = e.args[0] + if isinstance(e, KeyError): + reason += " is not the name of a Thing" + msg = f"Can't connect '{host.name}.{self.name}' because {reason}. " + if target is not ...: + msg += f"It was configured to connect to '{target}'. " + else: + msg += "It was not configured, and used the default. " + if self.default is not ...: + msg += f"The default is '{self.default}'." else: - raise ThingConnectionError(expected_sequence_msg % target) + msg += f"The default searches for Things by type: '{self.thing_type}'." + + raise ThingConnectionError(msg) from e def instance_get(self, obj: "Thing") -> ConnectedThings: r"""Supply the connected `.Thing`\ (s). @@ -231,13 +328,12 @@ def instance_get(self, obj: "Thing") -> ConnectedThings: return val -def thing_connection(default: str | Sequence[str] | None = None) -> Any: +def thing_connection(default: str | Iterable[str] | None | EllipsisType = ...) -> Any: r"""Declare a connection to another `.Thing` in the same server. - This function is a convenience wrapper around the `.ThingConnection` descriptor - class, and should be used in preference to using the descriptor directly. - The main reason to use the function is that it suppresses type errors when - using static type checkers such as `mypy` or `pyright`. + ``lt.thing_connection`` marks a class attribute as a connection to another + `.Thing` on the same server. This will be automatically supplied when the + server is started, based on the type hint and default value. In keeping with `.property` and `.setting`, the type of the attribute should be the type of the connected `.Thing`\ . For example: @@ -253,17 +349,85 @@ class ThingA(lt.Thing): ... class ThingB(lt.Thing): "A class that relies on ThingA." - thing_a: ThingA = lt.thing_connection("thing_a") + thing_a: ThingA = lt.thing_connection() + + This function is a convenience wrapper around the `.ThingConnection` descriptor + class, and should be used in preference to using the descriptor directly. + The main reason to use the function is that it suppresses type errors when + using static type checkers such as `mypy` or `pyright` (see note below). + + The type hint of a Thing Connection should be one of the following: + + * A `.Thing` subclass. An instance of this subclass will be returned when + the attribute is accessed. + * An optional `.Thing` subclass (e.g. ``MyThing | None``). This will either + return a ``MyThing`` instance or ``None``\ . + * A mapping of `str` to `.Thing` (e.g. ``Mapping[str, MyThing]``). This will + return a mapping of `.Thing` names to `.Thing` instances. The mapping + may be empty. + + Example: + + .. code-block:: python + + import labthings_fastapi as lt + + + class ThingA(lt.Thing): + "An example Thing." + + + class ThingB(lt.Thing): + "An example Thing with connections." + + thing_a: ThingA = lt.thing_connection() + maybe_thing_a: ThingA | None = lt.thing_connection() + all_things_a: Mapping[str, ThingA] = lt.thing_connection() + + @lt.thing_action + def show_connections(self) -> str: + "Tell someone about our connections." + self.thing_a # should always evaluate to a ThingA instance + self.maybe_thing_a # will be a ThingA instance or None + self.all_things_a # will a mapping of names to ThingA instances + return f"{self.thing_a=}, {self.maybe_thing_a=}, {self.all_things_a=}" + + The example above is very contrived, but shows how to apply the different types. + + If no default value is supplied, and no value is configured for the connection, + the server will attempt to find a `.Thing` that + matches the specified type when the server is started. If no matching `.Thing` + instances are found, the descriptor will return ``None`` or an empty mapping. + If that is not allowed by the type hint, the server will fail to start with + an error. + + The default value may be a string specifying a `.Thing` name, or a sequence of + strings (for connections that return mappings). In those cases, the relevant + `.Thing` will be returned from the server. If a name is given that either + doesn't correspond to a `.Thing` on the server, or is a `.Thing` that doesn't + match the type of this connection, the server will fail to start with an error. + + The default may also be ``None`` + which is appropriate when the type is optional or a mapping. If the type is + a `.Thing` subclass, a default value of ``None`` forces the connection to be + specified in configuration. + + :param default: The name(s) of the Thing(s) that will be connected by default. + If the default is omitted or set to ``...`` the server will attempt to find + a matching `.Thing` instance (or instances). A default value of `None` is + allowed if the connection is type hinted as optional. + :return: A `.ThingConnection` descriptor. + + Typing notes: In the example above, using `.ThingConnection` directly would assign an object with type ``ThingConnection[ThingA]`` to the attribute ``thing_a``, which is typed as ``ThingA``\ . This would cause a type error. Using - `.thing_connection` suppresses this error, as its return type is `Any`\ . + `.thing_connection` suppresses this error, as its return type is a`Any``\ . + + The use of ``Any`` or an alternative type-checking exemption seems to be + inevitable when implementing descriptors that are typed via attribute annotations, + and it is done by established libraries such as `pydantic`\ . - :param default: The name of the Thing that will be connected by default. - It is possible to omit this default or set it to ``None``\ , but if that - is done, it will cause an error unless the connection is explicitly made - in the server configuration. That is usually not desirable. - :return: A `.ThingConnection` instance. """ return ThingConnection(default=default) diff --git a/tests/test_thing_connection.py b/tests/test_thing_connection.py index 41455ff..b4306a9 100644 --- a/tests/test_thing_connection.py +++ b/tests/test_thing_connection.py @@ -11,68 +11,314 @@ class ThingOne(lt.Thing): """A class that will cause chaos if it can.""" - other_thing: "ThingTwo" = lt.thing_connection("thing_two") - multiple_things: Mapping[str, lt.Thing] = lt.thing_connection( - ["thing_one", "thing_two"] + other_thing: "ThingTwo" = lt.thing_connection() + n_things: "Mapping[str, ThingThree]" = lt.thing_connection() + optional_thing: "ThingThree | None" = lt.thing_connection() + + +class ThingTwo(lt.Thing): + """A class that relies on ThingOne.""" + + other_thing: ThingOne = lt.thing_connection() + + +class ThingN(lt.Thing): + """A class that emulates ThingOne and ThingTwo more generically.""" + + other_thing: "ThingN" = lt.thing_connection(None) + + +class ThingThree(lt.Thing): + """A Thing that has no other attributes.""" + + pass + + +class ThingThatMustBeConfigured(lt.Thing): + """A Thing that has a default that won't work.""" + + other_thing: lt.Thing = lt.thing_connection(None) + + +class Dummy: + """A dummy thing-like class.""" + + def __init__(self, name): + """Set the dummy Thing's name.""" + self.name = name + + +class Dummy1(Dummy): + """A subclass of Dummy.""" + + +class Dummy2(Dummy): + """A different subclass of Dummy.""" + + +class ThingWithManyConnections: + """A class with lots of ThingConnections. + + This class is not actually meant to be used - it is a host for + the thing_connection attributes. It's not a Thing, to simplify + testing. The "thing" types it depends on are also not Things, + again to simplify testing. + """ + + name = "thing" + + single_no_default: Dummy1 = lt.thing_connection() + optional_no_default: Dummy1 | None = lt.thing_connection() + multiple_no_default: Mapping[str, Dummy1] = lt.thing_connection() + + single_default_none: Dummy1 = lt.thing_connection(None) + optional_default_none: Dummy1 | None = lt.thing_connection(None) + multiple_default_none: Mapping[str, Dummy1] = lt.thing_connection(None) + + single_default_str: Dummy1 = lt.thing_connection("dummy_a") + optional_default_str: Dummy1 | None = lt.thing_connection("dummy_a") + multiple_default_str: Mapping[str, Dummy1] = lt.thing_connection("dummy_a") + + single_default_seq: Dummy1 = lt.thing_connection(["dummy_a", "dummy_b"]) + optional_default_seq: Dummy1 | None = lt.thing_connection(["dummy_a", "dummy_b"]) + multiple_default_seq: Mapping[str, Dummy1] = lt.thing_connection( + ["dummy_a", "dummy_b"] ) - @lt.thing_action - def say_hello(self) -> str: - """An example function.""" - return "Hello from thing_one." - @lt.thing_action - def ask_other_thing(self) -> str: - """Ask ThingTwo to say hello.""" - return self.other_thing.say_hello() +class ThingWithFutureConnection: + """A class with a ThingConnection in the future.""" + name = "thing" -class ThingTwo(lt.Thing): - """A class that relies on ThingOne.""" + single: "DummyFromTheFuture" = lt.thing_connection() + optional: "DummyFromTheFuture | None" = lt.thing_connection() + multiple: "Mapping[str, DummyFromTheFuture]" = lt.thing_connection() - other_thing: ThingOne = lt.thing_connection("thing_one") - @lt.thing_action - def say_hello(self) -> str: - """An example function.""" - return "Hello from thing_two." +class DummyFromTheFuture(Dummy): + """A subclass of the dummy Thing defined after the dependent class.""" - @lt.thing_action - def ask_other_thing(self) -> str: - """Ask ThingOne to say hello.""" - return self.other_thing.say_hello() +def dummy_things(names, cls=Dummy1): + """Turn a list or set of names into a dict of Things.""" + return {n: cls(n) for n in names} -class ThingN(lt.Thing): - """A class that emulates ThingOne and ThingTwo more generically.""" - other_thing: "ThingN" = lt.thing_connection() +def names_set(thing_or_mapping): + """Given a mapping or a Thing, return a set of names.""" + if thing_or_mapping is None: + return set() + if isinstance(thing_or_mapping, str): + return {thing_or_mapping} + else: + return {t.name for t in thing_or_mapping.values()} - @lt.thing_action - def say_hello(self) -> str: - """An example function.""" - return f"Hello from {self.name}." - @lt.thing_action - def ask_other_thing(self) -> str: - """Ask the other thing to say hello.""" - return self.other_thing.say_hello() +@pytest.fixture +def mixed_things(): + """A list of Things with two different types.""" + return { + **dummy_things({"thing1_a", "thing1_b"}, Dummy1), + **dummy_things({"thing2_a", "thing2_b"}, Dummy2), + } -class ThingOfWrongType(lt.Thing): - """A Thing that has no other attributes.""" +CONN_TYPES = ["single", "optional", "multiple"] +DEFAULTS = ["no_default", "default_none", "default_str", "default_seq"] + + +@pytest.mark.parametrize("conn_type", CONN_TYPES) +@pytest.mark.parametrize("default", DEFAULTS) +def test_type_analysis(conn_type, default): + """Check the type of things and thing connections is correctly determined.""" + attr = getattr(ThingWithManyConnections, f"{conn_type}_{default}") + + # All the attributes use the same type of Thing, Dummy1 + assert attr.thing_type == (Dummy1,) + assert attr.is_optional is (conn_type == "optional") + assert attr.is_mapping is (conn_type == "multiple") - pass +@pytest.mark.parametrize("conn_type", CONN_TYPES) +def test_type_analysis_strings(conn_type): + """Check connection types still work with stringified annotations.""" + attr = getattr(ThingWithFutureConnection, f"{conn_type}") -def test_type_analysis(): + # All the attributes use the same type of Thing, Dummy1 + assert attr.thing_type == (DummyFromTheFuture,) + assert attr.is_optional is (conn_type == "optional") + assert attr.is_mapping is (conn_type == "multiple") + + +def test_pick_things(mixed_things): + r"""Test the logic that picks things from the server. + + Note that ``_pick_things`` depends only on the ``thing_type`` of the connection, + not on whether it's optional or a mapping. Those are dealt with in ``connect``\ . + """ + attr = ThingWithManyConnections.single_no_default + + def picked_names(things, target): + return {t.name for t in attr._pick_things(things, target)} + + # If the target is None, we always get an empty list. + for names in [[], ["thing1_a"], ["thing1_a", "thing1_b"]]: + assert picked_names(dummy_things(names), None) == set() + + # If there are no other Things, picking by class returns nothing. + assert picked_names({}, ...) == set() + + # If there are other Things, they should be filtered by type. + for names1 in [[], ["thing1_a"], ["thing1_a", "thing1_b"]]: + for names2 in [[], ["thing2_a"], ["thing2_a", "thing2_b"]]: + mixed_things = { + **dummy_things(names1, Dummy1), + **dummy_things(names2, Dummy2), + } + assert picked_names(mixed_things, ...) == set(names1) + + # If a string is specified, it works when it exists and it's the right type. + for target in ["thing1_a", "thing1_b"]: + assert picked_names(mixed_things, target) == {target} + # If a sequence of strings is specified, it should also check existence and type. + # The targets below all exist and have the right type. + for target in [[], ["thing1_a"], ["thing1_a", "thing1_b"]]: + assert picked_names(mixed_things, target) == set(target) + # Any iterable will do - a set is not a sequence, but it is an iterable. + # This checks sets are OK as well. + for target in [set(), {"thing1_a"}, {"thing1_a", "thing1_b"}]: + assert picked_names(mixed_things, target) == target + + # Check for the error if we specify the wrong type (for string and sequence) + # Note that only one thing of the wrong type will still cause the error. + for target in ["thing2_a", ["thing2_a"], ["thing1_a", "thing2_a"]]: + with pytest.raises(ThingConnectionError) as excinfo: + picked_names(mixed_things, target) + assert "wrong type" in str(excinfo.value) + + # Check for a KeyError if we specify a missing Thing. This is converted to + # a ThingConnectionError by `connect`. + for target in ["something_else", {"thing1_a", "something_else"}]: + with pytest.raises(KeyError): + picked_names(mixed_things, target) + + # Check for a TypeError if the target is the wrong type. + with pytest.raises(TypeError): + picked_names(mixed_things, True) + + +def test_connect(mixed_things): + """Test connecting different attributes produces the right result""" + cls = ThingWithManyConnections # This is just to save typing! + + # A default of None means no things should be returned by default + # This is OK for optional connections and mappings, but not for + # connections typed as a Thing: these must always have a value. + for names in [set(), {"thing_a"}, {"thing_a", "thing_b"}]: + obj = cls() + cls.optional_default_none.connect(obj, dummy_things(names)) + assert obj.optional_default_none is None + cls.multiple_default_none.connect(obj, dummy_things(names)) + assert names_set(obj.multiple_default_none) == set() + # single should fail, as it requires a Thing + with pytest.raises(ThingConnectionError) as excinfo: + cls.single_default_none.connect(obj, dummy_things(names)) + assert "must be set" in str(excinfo.value) + + # We should be able to override this by giving names. + # Note that a sequence with one element and a single string are equivalent. + for target in ["thing1_a", ["thing1_a"]]: + obj = cls() + cls.single_default_none.connect(obj, mixed_things, target) + assert obj.single_default_none.name == "thing1_a" + cls.optional_default_none.connect(obj, mixed_things, target) + assert obj.optional_default_none.name == "thing1_a" + cls.multiple_default_none.connect(obj, mixed_things, target) + assert names_set(obj.multiple_default_none) == {"thing1_a"} + + # A default of `...` (i.e. no default) picks by class. + # Different types have different constraints on how many are allowed. + + # If there are no matching Things, optional and multiple are OK, + # but a single connection fails, as it can't be None. + no_matches = {n: Dummy2(n) for n in ["one", "two"]} + obj = cls() + with pytest.raises(ThingConnectionError) as excinfo: + cls.single_no_default.connect(obj, no_matches) + assert "no matching Thing" in str(excinfo.value) + cls.optional_no_default.connect(obj, no_matches) + assert obj.optional_no_default is None + cls.multiple_no_default.connect(obj, no_matches) + assert obj.multiple_no_default == {} + + # If there's exactly one matching Thing, everything works. + match = Dummy1("three") + one_match = {"three": match, **no_matches} + obj = cls() + cls.single_no_default.connect(obj, one_match) + assert obj.single_no_default is match + cls.optional_no_default.connect(obj, one_match) + assert obj.optional_no_default is match + cls.multiple_no_default.connect(obj, one_match) + assert obj.multiple_no_default == {"three": match} + + # If we have more than one match, only the multiple connection + # is OK. + match2 = Dummy1("four") + two_matches = {"four": match2, **one_match} + obj = cls() + with pytest.raises(ThingConnectionError) as excinfo: + cls.single_no_default.connect(obj, two_matches) + assert "multiple Things" in str(excinfo.value) + assert "Things by type" in str(excinfo.value) + with pytest.raises(ThingConnectionError) as excinfo: + cls.optional_no_default.connect(obj, two_matches) + assert "multiple Things" in str(excinfo.value) + assert "Things by type" in str(excinfo.value) + cls.multiple_no_default.connect(obj, two_matches) + assert obj.multiple_no_default == {"three": match, "four": match2} + + # _pick_things raises KeyErrors for invalid names. + # Check KeyErrors are turned back into ThingConnectionErrors + obj = cls() + with pytest.raises(ThingConnectionError) as excinfo: + cls.single_default_str.connect(obj, mixed_things) + assert "not the name of a Thing" in str(excinfo.value) + assert f"{obj.name}.single_default_str" in str(excinfo.value) + assert "not configured, and used the default" in str(excinfo.value) + # The error message changes if a target is specified. + obj = cls() + with pytest.raises(ThingConnectionError) as excinfo: + cls.single_default_str.connect(obj, mixed_things, "missing") + assert "not the name of a Thing" in str(excinfo.value) + assert f"{obj.name}.single_default_str" in str(excinfo.value) + assert "configured to connect to 'missing'" in str(excinfo.value) + + +def test_readonly(): + """Test that thing connections are read-only.""" + obj = ThingWithManyConnections() + with pytest.raises(AttributeError, match="read-only"): + obj.single_default_none = Dummy("name") + + +# The tests below use real Things and a real ThingServer to do more +# realistic tests. These are not as exhaustive as the tests above, +# but I think there's no harm in taking both approaches. +def test_type_analysis_thingone(): """Check the correct properties are inferred from the type hints.""" assert ThingOne.other_thing.is_optional is False assert ThingOne.other_thing.is_mapping is False - assert ThingOne.other_thing.thing_type is ThingTwo - assert ThingOne.multiple_things.is_optional is False - assert ThingOne.multiple_things.is_mapping is True - assert ThingOne.multiple_things.thing_type is lt.Thing + assert ThingOne.other_thing.thing_type == (ThingTwo,) + + assert ThingOne.n_things.is_optional is False + assert ThingOne.n_things.is_mapping is True + assert ThingOne.n_things.thing_type == (ThingThree,) + + assert ThingOne.optional_thing.is_optional is True + assert ThingOne.optional_thing.is_mapping is False + assert ThingOne.optional_thing.thing_type == (ThingThree,) CONNECTIONS = { @@ -89,7 +335,7 @@ def test_type_analysis(): (ThingN, ThingN, CONNECTIONS), ], ) -def test_thing_connection(cls_1, cls_2, connections) -> None: +def test_circular_connection(cls_1, cls_2, connections) -> None: """Check that two things can connect to each other. Note that this test includes a circular dependency, which is fine. @@ -101,48 +347,58 @@ def test_thing_connection(cls_1, cls_2, connections) -> None: thing_one = server.add_thing("thing_one", cls_1) thing_two = server.add_thing("thing_two", cls_2) things = [thing_one, thing_two] - numbers = ["one", "two"] if connections is not None: server.thing_connections = connections - # Check the things say hello correctly - for thing, num in zip(things, numbers, strict=True): - assert thing.say_hello() == f"Hello from thing_{num}." - # Check the connections don't work initially, because they aren't connected for thing in things: with pytest.raises(ThingNotConnectedError): - thing.ask_other_thing() + _ = thing.other_thing - with TestClient(server.app) as client: + with TestClient(server.app) as _: # The things should be connected as the server is now running + for thing, other in zip(things, reversed(things), strict=True): + assert thing.other_thing is other - # Check the things are connected, calling actions directly - for thing, num in zip(things, reversed(numbers), strict=True): - assert thing.ask_other_thing() == f"Hello from thing_{num}." - # Check the same happens over "HTTP" (i.e. the TestClient) - for num, othernum in zip(numbers, reversed(numbers), strict=True): - thing = lt.ThingClient.from_url(f"/thing_{num}/", client=client) - assert thing.ask_other_thing() == f"Hello from thing_{othernum}." - assert thing.say_hello() == f"Hello from thing_{num}." +def connectionerror_starting_server(server): + """Attempt to start a server, and return the error as a string.""" + with pytest.RaisesGroup(ThingConnectionError) as excinfo: + # Creating a TestClient starts the server + with TestClient(server.app): + pass + # excinfo contains an ExceptionGroup because TestClient runs in a + # task group, hence the use of RaisesGroup and the `.exceptions[0]` + # below. + return str(excinfo.value.exceptions[0]) @pytest.mark.parametrize( ("connections", "error"), [ - ({}, "no default"), - ({"thing_one": {"other_thing": "non_existent_thing"}}, "does not exist"), - ({"thing_one": {"other_thing": ("thing_one", "thing_one")}}, "single Thing"), - ({"thing_one": {"other_thing": "wrong_type_thing"}}, "not of type"), - ({"thing_one": {"other_thing": "thing_one"}}, None), + ({}, "must be set"), + ({"thing_one": {"other_thing": "non_thing"}}, "not the name of a Thing"), + ({"thing_one": {"other_thing": "thing_three"}}, "wrong type"), + ( + { + "thing_one": {"other_thing": "thing_one"}, + "thing_two": {"other_thing": "thing_one"}, + }, + None, + ), ], ) -def test_connections_no_default(connections, error): - """Check a connection with no default and no config errors out.""" +def test_connections_none_default(connections, error): + """Check error conditions for a connection with a default of None. + + Note that we only catch the first error - that's why we only need + to specify connections for 'thing_two' in the last case - because + that's the only one where 'thing_one' connects successfully. + """ server = lt.ThingServer() thing_one = server.add_thing("thing_one", ThingN) - server.add_thing("wrong_type_thing", ThingOfWrongType) + server.add_thing("thing_two", ThingN) + server.add_thing("thing_three", ThingThree) server.thing_connections = connections @@ -151,11 +407,39 @@ def test_connections_no_default(connections, error): assert thing_one.other_thing is thing_one return - with pytest.RaisesGroup(ThingConnectionError) as excinfo: - # Creating a TestClient should activate the connections - with TestClient(server.app): - pass - # excinfo contains an ExceptionGroup because TestClient runs in a - # task group, hence the use of RaisesGroup and the `.exceptions[0]` - # below. - assert error in str(excinfo.value.exceptions[0]) + assert error in connectionerror_starting_server(server) + + +def test_optional_and_empty(): + """Check that an optional or mapping connection can be None/empty.""" + server = lt.ThingServer() + thing_one = server.add_thing("thing_one", ThingOne) + _thing_two = server.add_thing("thing_two", ThingTwo) + + with TestClient(server.app): + assert thing_one.optional_thing is None + assert len(thing_one.n_things) == 0 + + +def test_mapping_and_multiple(): + """Check that a mapping connection can pick up several Things. + + This also tests the expected error if multiple things match a + single connection. + """ + server = lt.ThingServer() + thing_one = server.add_thing("thing_one", ThingOne) + _thing_two = server.add_thing("thing_two", ThingTwo) + for i in range(3): + server.add_thing(f"thing_{i + 3}", ThingThree) + + # Attempting to start the server should fail, because + # thing_one.optional_thing will match multiple ThingThree instances. + assert "multiple Things" in connectionerror_starting_server(server) + + # Set optional thing to one specific name and it will start OK. + server.thing_connections = {"thing_one": {"optional_thing": "thing_3"}} + + with TestClient(server.app): + assert thing_one.optional_thing.name == "thing_3" + assert names_set(thing_one.n_things) == {f"thing_{i + 3}" for i in range(3)} From 739372c74e61e5cb4e1474992f588e6a97bdf467 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 25 Sep 2025 23:50:09 +0100 Subject: [PATCH 13/29] Removed unnecessary override. FunctionalProperty was overriding `value_type` provided by FieldTypedBaseProperty. The override wasn't typed properly. I've removed it - there is no need to override the base implementation. --- src/labthings_fastapi/properties.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 1e31d38..8afde31 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -616,14 +616,6 @@ def __init__( self._fset: ValueSetter | None = None self.readonly: bool = True - @builtins.property - def value_type(self) -> type[Value]: - """The type of this descriptor's value. - - :return: the type of the descriptor's value. - """ - return self._type - @builtins.property def fget(self) -> ValueGetter: # noqa: DOC201 """The getter function.""" From 446462404689b2940a5d442a40e56c4609d611e7 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 25 Sep 2025 23:52:01 +0100 Subject: [PATCH 14/29] Typing fixes. I had hoped not to need to ignore these. It would probably be possible if I made ThingConnection non-generic, but I think it is worth keeping that feature: it allows us to specify a subscripted class, if for whatever reason it's better to do that than use a type hint on the attribute. I have added explanation in the docstring as to why I have ignored type checking, and why I think it's the right thing to do. --- src/labthings_fastapi/thing_connections.py | 30 +++++++++++++++++----- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/labthings_fastapi/thing_connections.py b/src/labthings_fastapi/thing_connections.py index 6acec8a..8d712df 100644 --- a/src/labthings_fastapi/thing_connections.py +++ b/src/labthings_fastapi/thing_connections.py @@ -43,7 +43,7 @@ def say_hello(self) -> str: from types import EllipsisType, NoneType, UnionType from typing import Any, Generic, TypeVar, TYPE_CHECKING, Union, get_args, get_origin -from collections.abc import Mapping, Iterable +from collections.abc import Mapping, Iterable, Sequence from weakref import ReferenceType, WeakKeyDictionary, ref, WeakValueDictionary from .base_descriptor import FieldTypedBaseDescriptor from .exceptions import ThingNotConnectedError, ThingConnectionError @@ -125,7 +125,7 @@ def __init__( ] = WeakKeyDictionary() @property - def thing_type(self) -> tuple[type["Thing"], ...]: + def thing_type(self) -> tuple[type, ...]: r"""The `.Thing` subclass(es) returned by this connection. A tuple is returned to allow for optional thing connections that @@ -178,7 +178,7 @@ def _pick_things( self, things: "Mapping[str, Thing]", target: str | Iterable[str] | None | EllipsisType, - ) -> "Iterable[Thing]": + ) -> "Sequence[Thing]": r"""Pick the Things we should connect to from a list. This function is used internally by `.ThingConnection.connect` to choose @@ -313,19 +313,35 @@ def instance_get(self, obj: "Thing") -> ConnectedThings: :return: the `.Thing` instance(s) connected. :raises ThingNotConnectedError: if the ThingConnection has not yet been set up. + + Typing notes: + + This must be annotated as ``ConnectedThings`` which is the type variable + corresponding to the type of this connection. The type determined + at runtime will be within the upper bound of ``ConnectedThings`` but it + would be possible for ``ConnectedThings`` to be more specific. + + In general, types determined at runtime may conflict with generic types, + and at least for this class the important thing is that types determined + at runtime match the attribute annotations, which is tested in unit tests. + + The return statements here consequently have their types ignored. + """ msg = f"{self.name} has not been connected to a Thing yet." try: val = self._things[obj] except KeyError as e: raise ThingNotConnectedError(msg) from e - # Note that ReferenceError is deliberately not handled: the Thing - # referred to by thing_ref should exist until the server has shut down. if isinstance(val, ReferenceType): - return val() + thing = val() + if thing is not None: + return thing # type: ignore[return-value] + else: + raise ReferenceError("A connected thing was garbage collected.") else: # This works for None or for WeakValueDictionary() - return val + return val # type: ignore[return-value] def thing_connection(default: str | Iterable[str] | None | EllipsisType = ...) -> Any: From 7a266e709d2b474d2594db13740a440baa484f15 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 25 Sep 2025 23:53:29 +0100 Subject: [PATCH 15/29] Update tests to take account of different type evaluation. Types are now evaluated lazily, but we check for the existence of a type hint in __set_name__. This used to happen only for `DataProperty` but now happens for `BaseProperty` too, so I needed to adjust some tests. --- tests/test_property.py | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/tests/test_property.py b/tests/test_property.py index 8f966d8..a401937 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -155,22 +155,19 @@ def test_baseproperty_type_and_model(): `pydantic.RootModel`. """ - class Example: - prop = tp.BaseProperty() + with pytest.raises(tp.MissingTypeError): - prop = Example.prop + class Example: + prop = tp.BaseProperty() - # By default, we have no type so `.type` errors. - with pytest.raises(tp.MissingTypeError): - _ = prop.value_type - with pytest.raises(tp.MissingTypeError): - _ = prop.model + class Example: + prop: "str | None" = tp.BaseProperty() - # Once _type is set, these should both work. - prop._type = str | None - assert str(prop.value_type) == "str | None" - assert issubclass(prop.model, pydantic.RootModel) - assert str(prop.model.model_fields["root"].annotation) == "str | None" + assert isinstance(None, Example.prop.value_type) + assert isinstance("test", Example.prop.value_type) + assert str(Example.prop.value_type) == "str | None" + assert issubclass(Example.prop.model, pydantic.RootModel) + assert str(Example.prop.model.model_fields["root"].annotation) == "str | None" def test_baseproperty_type_and_model_pydantic(): @@ -180,19 +177,15 @@ def test_baseproperty_type_and_model_pydantic(): type is a BaseModel instance. """ - class Example: - prop = tp.BaseProperty() - - prop = Example.prop - class MyModel(pydantic.BaseModel): foo: str bar: int - # Once _type is set, these should both work. - prop._type = MyModel - assert prop.value_type is MyModel - assert prop.model is MyModel + class Example: + prop: MyModel = tp.BaseProperty() + + assert Example.prop.value_type is MyModel + assert Example.prop.model is MyModel def test_baseproperty_add_to_fastapi(): From dbcbe72f8cafc73e49e1267129bf9ca0faaf3541 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 25 Sep 2025 23:54:05 +0100 Subject: [PATCH 16/29] Check ReferenceError is raised if a Thing gets deleted. This tests the remaining 1 line that was uncovered. --- tests/test_thing_connection.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_thing_connection.py b/tests/test_thing_connection.py index b4306a9..90bf7bc 100644 --- a/tests/test_thing_connection.py +++ b/tests/test_thing_connection.py @@ -1,6 +1,7 @@ """Test the thing_connection module.""" from collections.abc import Mapping +import gc import pytest import labthings_fastapi as lt from fastapi.testclient import TestClient @@ -303,6 +304,17 @@ def test_readonly(): obj.single_default_none = Dummy("name") +def test_referenceerror(): + """Check an error is raised by premature deletion.""" + obj = ThingWithManyConnections() + things = {"name": Dummy1("name")} + ThingWithManyConnections.single_no_default.connect(obj, things) + del things + gc.collect() + with pytest.raises(ReferenceError): + _ = obj.single_no_default + + # The tests below use real Things and a real ThingServer to do more # realistic tests. These are not as exhaustive as the tests above, # but I think there's no harm in taking both approaches. From 2260d64c1caac4b8bad63073fd21be5e5db71a77 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Thu, 25 Sep 2025 23:55:04 +0100 Subject: [PATCH 17/29] Add comment to #type: ignore --- src/labthings_fastapi/thing_connections.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/thing_connections.py b/src/labthings_fastapi/thing_connections.py index 8d712df..0480816 100644 --- a/src/labthings_fastapi/thing_connections.py +++ b/src/labthings_fastapi/thing_connections.py @@ -336,12 +336,12 @@ def instance_get(self, obj: "Thing") -> ConnectedThings: if isinstance(val, ReferenceType): thing = val() if thing is not None: - return thing # type: ignore[return-value] + return thing # type: ignore[return-value] (see docstring) else: raise ReferenceError("A connected thing was garbage collected.") else: # This works for None or for WeakValueDictionary() - return val # type: ignore[return-value] + return val # type: ignore[return-value] (see Typing notes in docstring) def thing_connection(default: str | Iterable[str] | None | EllipsisType = ...) -> Any: From 345134401bd9601e3e9d6456a7a96fe9f04c6ca5 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 6 Oct 2025 23:18:26 +0100 Subject: [PATCH 18/29] Explicitly don't support forward references in type subscripts. typing.get_type_hints automatically resolves forward references using sensible values for locals and globals. This is used to permit forward references in field-typed descriptors. There's no corresponding way to resolve forward references in type subscripts, so for now I have made this an error - descriptors that are subscripted may not be subscripted with a string. Given that I don't anticipate type subscripts will be used much, this is probably not a major limitation. It could be fixed by implementing our own type evaluator based on `get_type_hints` but I would prefer not to do this if I can avoid it. --- src/labthings_fastapi/base_descriptor.py | 32 ++++++------------------ 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/src/labthings_fastapi/base_descriptor.py b/src/labthings_fastapi/base_descriptor.py index 3acca1f..c123e34 100644 --- a/src/labthings_fastapi/base_descriptor.py +++ b/src/labthings_fastapi/base_descriptor.py @@ -432,6 +432,12 @@ class MyThing(Thing): # with a subscripted type. It is not available during __init__, which # is why we check for it here. self._type = typing.get_args(self.__orig_class__)[0] + if isinstance(self._type, typing.ForwardRef): + raise MissingTypeError( + f"{owner}.{name} is a subscripted descriptor, where the " + f"subscript is a forward reference ({self._type}). Forward " + "references are not supported as subscripts." + ) # Check for annotations on the parent class field_annotation = inspect.get_annotations(owner).get(name, None) @@ -477,7 +483,7 @@ def value_type(self) -> type[Value]: :raises MissingTypeError: if the type is None, not resolvable, or not specified. """ self.assert_set_name_called() - if self._unevaluated_type_hint is not None: + if self._type is None and self._unevaluated_type_hint is not None: # We have a forward reference, so we need to resolve it. if self._owner is None: raise MissingTypeError( @@ -533,30 +539,6 @@ def value_type(self) -> type[Value]: ) -def get_class_attribute_annotation( - cls: type, name: str -) -> tuple[type | str, type] | tuple[None, None]: - """Retrieve the type annotation for an attribute of a class. - - This function retrieves the type annotation for an attribute of a class, - if one exists. It uses `inspect.get_annotations` to retrieve the - annotation without resolving forward references, and it searches - the class and its parents by traversing the method resolution order. - - :param cls: The class to inspect. - :param name: The name of the attribute whose annotation is required. - :return: A tuple of the annotation and the class on which it's defined (which - may be a parent of the class supplied), or ``(None, None)`` if there is - no annotation. A tuple is supplied so unpacking of the return value - will always work. - """ - for base in inspect.getmro(cls): - annotations = inspect.get_annotations(base) - if name in annotations: - return annotations[name], base - return (None, None) - - def get_class_attribute_docstrings(cls: type) -> Mapping[str, str]: """Retrieve docstrings for the attributes of a class. From 0e6908cf56a58d3ea9b2a770f2742c6ae648d477 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 6 Oct 2025 23:18:51 +0100 Subject: [PATCH 19/29] Add full testing for FieldTypedBaseDescriptor. --- tests/test_base_descriptor.py | 164 ++++++++++++++++++++++++++++++++++ 1 file changed, 164 insertions(+) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index 34e3763..2973eb4 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -1,11 +1,14 @@ +import gc import pytest from labthings_fastapi.base_descriptor import ( BaseDescriptor, + FieldTypedBaseDescriptor, DescriptorNotAddedToClassError, DescriptorAddedToClassTwiceError, get_class_attribute_docstrings, ) from .utilities import raises_or_is_caused_by +from labthings_fastapi.exceptions import MissingTypeError, InconsistentTypeError class MockProperty(BaseDescriptor[str]): @@ -284,3 +287,164 @@ class SecondExampleClass: assert "prop" in str(excinfo.value) assert "FirstExampleClass" in str(excinfo.value) assert "SecondExampleClass" in str(excinfo.value) + + +class CustomType: + """A custom datatype.""" + + pass + + +class FieldTypedExample: + """An example with field-typed descriptors.""" + + int_or_str_prop: int | str = FieldTypedBaseDescriptor() + int_or_str_subscript = FieldTypedBaseDescriptor[int | str]() + int_or_str_stringified: "int | str" = FieldTypedBaseDescriptor() + customprop: CustomType = FieldTypedBaseDescriptor() + customprop_subscript = FieldTypedBaseDescriptor[CustomType]() + futureprop: "FutureType" = FieldTypedBaseDescriptor() + + +class FutureType: + """A custom datatype, defined after the descriptor.""" + + pass + + +@pytest.mark.parametrize( + ("name", "value_type"), + [ + ("int_or_str_prop", int | str), + ("int_or_str_subscript", int | str), + ("int_or_str_stringified", int | str), + ("customprop", CustomType), + ("customprop_subscript", CustomType), + ("futureprop", FutureType), + ], +) +def test_fieldtyped_definition(name, value_type): + """Test that field-typed descriptors pick up their type correctly.""" + prop = getattr(FieldTypedExample, name) + assert prop.name == name + assert prop.value_type == value_type + + +def test_fieldtyped_missingtype(): + """Check the right error is raised when no type can be found.""" + with pytest.raises(MissingTypeError) as excinfo: + + class Example2: + field2 = FieldTypedBaseDescriptor() + + msg = str(excinfo.value) + assert msg.startswith("No type hint was found") + # We check the field name is included, because the exception will + # arise from the end of the class definition, rather than the line + # where the field is defined. + assert "field2" in msg + + # This one defines OK, but should error when we access its type. + # Note that Ruff already spots the bad forward reference, hence the + # directive to ignore F821. + class Example3: + field3: "BadForwardReference" = FieldTypedBaseDescriptor() # noqa: F821 + field4: "int" = FieldTypedBaseDescriptor() + field5: "int" = FieldTypedBaseDescriptor() + + with pytest.raises(MissingTypeError) as excinfo: + _ = Example3.field3.value_type + + msg = str(excinfo.value) + assert "resolve forward ref" in msg + assert "field3" in msg + + # If we try to resolve a forward reference and the owner is None, it + # should raise an error. + # I don't see how this could happen in practice, _owner is always + # set if we find a forward reference. + # We force this error condition by manually setting _owner to None + Example3.field4._owner = None + + with pytest.raises(MissingTypeError) as excinfo: + _ = Example3.field4.value_type + + msg = str(excinfo.value) + assert "resolve forward ref" in msg + assert "wasn't saved" in msg + assert "field4" in msg + + # We re-use field4 but manually set _type and _unevaluated_type_hint + # to None, to test the catch-all error + Example3.field4._unevaluated_type_hint = None + + with pytest.raises(MissingTypeError): + _ = Example3.field4.value_type + + msg = str(excinfo.value) + assert "bug in LabThings" in msg + assert "caught before now" in msg + assert "field4" in msg + + # If the class is finalised before we evaluate type hints, we should + # get a MissingTypeError. This probably only happens on dynamically + # generated classes, and I think it's unlikely we'd dynamically generate + # Thing subclasses in a way that they go out of scope. + prop = Example3.field5 + del Example3 + gc.collect() + + with pytest.raises(MissingTypeError) as excinfo: + _ = prop.value_type + + msg = str(excinfo.value) + assert "resolve forward ref" in msg + assert "garbage collected" in msg + assert "field5" in msg + + # Rather than roll my own evaluator for forward references, we just + # won't support forward references in subscripted types for now. + with pytest.raises(MissingTypeError) as excinfo: + + class Example4: + field6 = FieldTypedBaseDescriptor["str"]() + + msg = str(excinfo.value) + assert "forward reference" in msg + assert "not supported as subscripts" + assert "field6" in msg + + +def test_mismatched_types(): + """Check two type hints that don't match raises an error.""" + with pytest.raises(InconsistentTypeError): + + class Example3: + field: int = FieldTypedBaseDescriptor[str]() + + +def test_double_specified_types(): + """Check two type hints that match are allowed. + + This is a very odd thing to do, but it feels right to allow + it, provided the types are an exact match. + """ + + class Example4: + field: int | None = FieldTypedBaseDescriptor[int | None]() + + assert Example4.field.value_type == int | None + + +def test_stringified_vs_unstringified_mismatch(): + """Test that string type hints don't match non-string ones. + + This behaviour may change in the future - but this test is here + to make sure that, if it does, we are changing it deliberately. + If a descriptor is typed using both a subscript and a field + annotation, they should match - + """ + with pytest.raises(InconsistentTypeError): + + class Example5: + field: "int" = FieldTypedBaseDescriptor[int]() From 79c08fbcef764e6a3839e54cecbdb92cff9750f2 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Mon, 6 Oct 2025 23:23:47 +0100 Subject: [PATCH 20/29] Mark dependencies as deprecated. --- docs/source/dependencies/dependencies.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/source/dependencies/dependencies.rst b/docs/source/dependencies/dependencies.rst index 88426e9..7d468e1 100644 --- a/docs/source/dependencies/dependencies.rst +++ b/docs/source/dependencies/dependencies.rst @@ -3,11 +3,19 @@ Dependencies ============ +.. warning:: + + The use of dependencies is now deprecated. See `.thing_connection` and `.ThingServerInterface` for a more intuitive way to access that functionality. + LabThings makes use of the powerful "dependency injection" mechanism in FastAPI. You can see the `FastAPI documentation`_ for more information. In brief, FastAPI dependencies are annotated types that instruct FastAPI to supply certain function arguments automatically. This removes the need to set up resources at the start of a function, and ensures everything the function needs is declared and typed clearly. The most common use for dependencies in LabThings is where an action needs to make use of another `.Thing` on the same `.ThingServer`. Inter-Thing dependencies ------------------------ +.. warning:: + + These dependencies are deprecated - see `.thing_connection` instead. + Simple actions depend only on their input parameters and the `.Thing` on which they are defined. However, it's quite common to need something else, for example accessing another `.Thing` instance on the same LabThings server. There are two important principles to bear in mind here: * Other `.Thing` instances should be accessed using a `.DirectThingClient` subclass if possible. This creates a wrapper object that should work like a `.ThingClient`, meaning your code should work either on the server or in a client script. This makes the code much easier to debug. From 997f1b656e87c178a1c494a064f4764045c13576 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 7 Oct 2025 14:45:36 +0100 Subject: [PATCH 21/29] Fix broken test An exception was being ignored because of a missing `as excinfo` in a `pytest.raises` block. --- tests/test_base_descriptor.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index 2973eb4..a6b33c2 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -377,8 +377,9 @@ class Example3: # We re-use field4 but manually set _type and _unevaluated_type_hint # to None, to test the catch-all error Example3.field4._unevaluated_type_hint = None + Example3.field4._type = None - with pytest.raises(MissingTypeError): + with pytest.raises(MissingTypeError) as excinfo: _ = Example3.field4.value_type msg = str(excinfo.value) From a5d61d2d5f1b060daae26b1a2a094b6efb0cbdbc Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 7 Oct 2025 14:49:29 +0100 Subject: [PATCH 22/29] Fix type ignore comments --- src/labthings_fastapi/thing_connections.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/thing_connections.py b/src/labthings_fastapi/thing_connections.py index 0480816..0cf84d7 100644 --- a/src/labthings_fastapi/thing_connections.py +++ b/src/labthings_fastapi/thing_connections.py @@ -336,12 +336,14 @@ def instance_get(self, obj: "Thing") -> ConnectedThings: if isinstance(val, ReferenceType): thing = val() if thing is not None: - return thing # type: ignore[return-value] (see docstring) + return thing # type: ignore[return-value] + # See docstring for an explanation of the type ignore directives. else: raise ReferenceError("A connected thing was garbage collected.") else: # This works for None or for WeakValueDictionary() - return val # type: ignore[return-value] (see Typing notes in docstring) + return val # type: ignore[return-value] + # See docstring for an explanation of the type ignore directives. def thing_connection(default: str | Iterable[str] | None | EllipsisType = ...) -> Any: From b0c12c6e93bc4483c1e4e5af916ecf019fb4b6ff Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 7 Oct 2025 14:52:09 +0100 Subject: [PATCH 23/29] Fix docstring --- src/labthings_fastapi/thing_connections.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/labthings_fastapi/thing_connections.py b/src/labthings_fastapi/thing_connections.py index 0cf84d7..9431980 100644 --- a/src/labthings_fastapi/thing_connections.py +++ b/src/labthings_fastapi/thing_connections.py @@ -313,6 +313,8 @@ def instance_get(self, obj: "Thing") -> ConnectedThings: :return: the `.Thing` instance(s) connected. :raises ThingNotConnectedError: if the ThingConnection has not yet been set up. + :raises ReferenceError: if a connected Thing no longer exists (should not + ever happen in normal usage). Typing notes: From e9a0e2914c38b026e4a8e101b4754230e57d3c23 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 7 Oct 2025 15:19:12 +0100 Subject: [PATCH 24/29] Pass Thing Connection config through `add_thing`. This adds an argument to `ThingServer.add_thing` that captures configuration for thing connections. --- src/labthings_fastapi/server/__init__.py | 12 ++++++++++-- tests/test_thing_connection.py | 20 +++++++++++++++----- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 5dc7e22..4552935 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -15,7 +15,7 @@ from fastapi.middleware.cors import CORSMiddleware from anyio.from_thread import BlockingPortal from contextlib import asynccontextmanager, AsyncExitStack -from collections.abc import Mapping, Sequence +from collections.abc import Iterable, Mapping, Sequence from types import MappingProxyType from ..exceptions import ThingConnectionError as ThingConnectionError @@ -84,7 +84,7 @@ def __init__(self, settings_folder: Optional[str] = None) -> None: self.blob_data_manager.attach_to_app(self.app) self.add_things_view_to_app() self._things: dict[str, Thing] = {} - self.thing_connections: Mapping[str, Mapping[str, str | Sequence[str]]] = {} + self.thing_connections: dict[str, Mapping[str, str | Iterable[str] | None]] = {} self.blocking_portal: Optional[BlockingPortal] = None self.startup_status: dict[str, str | dict] = {"things": {}} global _thing_servers # noqa: F824 @@ -158,6 +158,7 @@ def add_thing( thing_subclass: type[ThingSubclass], args: Sequence[Any] | None = None, kwargs: Mapping[str, Any] | None = None, + thing_connections: Mapping[str, str | Iterable[str] | None] | None = None, ) -> ThingSubclass: r"""Add a thing to the server. @@ -173,6 +174,11 @@ def add_thing( ``thing_subclass``\ . :param kwargs: keyword arguments to pass to the constructor of ``thing_subclass``\ . + :param thing_connections: a mapping that sets up the `.thing_connection`\ s. + Keys are the names of attributes of the `.Thing` and the values are + the name(s) of the `.Thing`\ (s) you'd like to connect. If this is left + at its default, the connections will use their default behaviour, usually + automatically connecting to a `.Thing` of the right type. :returns: the instance of ``thing_subclass`` that was created and added to the server. There is no need to retain a reference to this, as it @@ -212,6 +218,8 @@ def add_thing( thing_server_interface=interface, ) # type: ignore[misc] self._things[name] = thing + if thing_connections is not None: + self.thing_connections[name] = thing_connections thing.attach_to_server( server=self, ) diff --git a/tests/test_thing_connection.py b/tests/test_thing_connection.py index 90bf7bc..f1ff483 100644 --- a/tests/test_thing_connection.py +++ b/tests/test_thing_connection.py @@ -342,7 +342,7 @@ def test_type_analysis_thingone(): @pytest.mark.parametrize( ("cls_1", "cls_2", "connections"), [ - (ThingOne, ThingTwo, None), + (ThingOne, ThingTwo, {}), (ThingOne, ThingTwo, CONNECTIONS), (ThingN, ThingN, CONNECTIONS), ], @@ -356,11 +356,13 @@ def test_circular_connection(cls_1, cls_2, connections) -> None: the LabThings server. """ server = lt.ThingServer() - thing_one = server.add_thing("thing_one", cls_1) - thing_two = server.add_thing("thing_two", cls_2) + thing_one = server.add_thing( + "thing_one", cls_1, thing_connections=connections.get("thing_one", {}) + ) + thing_two = server.add_thing( + "thing_two", cls_2, thing_connections=connections.get("thing_one", {}) + ) things = [thing_one, thing_two] - if connections is not None: - server.thing_connections = connections # Check the connections don't work initially, because they aren't connected for thing in things: @@ -455,3 +457,11 @@ def test_mapping_and_multiple(): with TestClient(server.app): assert thing_one.optional_thing.name == "thing_3" assert names_set(thing_one.n_things) == {f"thing_{i + 3}" for i in range(3)} + + +def test_connections_in_server(): + r"Check that ``thing_connections`` is correctly remembered from ``add_thing``\ ." + server = lt.ThingServer() + thing_one_connections = {"other_thing": "thing_name"} + server.add_thing("thing_one", ThingOne, thing_connections=thing_one_connections) + assert server.thing_connections["thing_one"] is thing_one_connections From 602ccf1fb66c1a782660399763c99cdc9f982446 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 7 Oct 2025 15:21:00 +0100 Subject: [PATCH 25/29] Allow thing connections to be specified in config. Pass thing_connections through from configuration. --- src/labthings_fastapi/server/__init__.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 4552935..0af0ec9 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -375,5 +375,6 @@ def server_from_config(config: dict) -> ThingServer: thing_subclass=cls, args=thing.get("args", ()), kwargs=thing.get("kwargs", {}), + thing_connections=thing.get("thing_connections", {}), ) return server From be6fc964d58528ec7d58d061699d120b90a11996 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 7 Oct 2025 15:29:46 +0100 Subject: [PATCH 26/29] Spelling fix --- tests/test_base_descriptor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index a6b33c2..5562546 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -374,7 +374,7 @@ class Example3: assert "wasn't saved" in msg assert "field4" in msg - # We re-use field4 but manually set _type and _unevaluated_type_hint + # We reuse field4 but manually set _type and _unevaluated_type_hint # to None, to test the catch-all error Example3.field4._unevaluated_type_hint = None Example3.field4._type = None From 04499c16bbfbbe087d9c79c54d71b0bf34d40aec Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 7 Oct 2025 21:42:45 +0100 Subject: [PATCH 27/29] Fix a typo in test code. This was causing two of the tests to fail. --- tests/test_thing_connection.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_thing_connection.py b/tests/test_thing_connection.py index f1ff483..05b71ea 100644 --- a/tests/test_thing_connection.py +++ b/tests/test_thing_connection.py @@ -360,7 +360,7 @@ def test_circular_connection(cls_1, cls_2, connections) -> None: "thing_one", cls_1, thing_connections=connections.get("thing_one", {}) ) thing_two = server.add_thing( - "thing_two", cls_2, thing_connections=connections.get("thing_one", {}) + "thing_two", cls_2, thing_connections=connections.get("thing_two", {}) ) things = [thing_one, thing_two] From cc3cb27fb6b7f8b3a78f18c70be3cf5ee0276796 Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 7 Oct 2025 22:29:49 +0100 Subject: [PATCH 28/29] Add a documentation page on thing connections. --- docs/source/dependencies/dependencies.rst | 4 +- docs/source/index.rst | 1 + docs/source/thing_connections.rst | 89 +++++++++++++++++++++++ 3 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 docs/source/thing_connections.rst diff --git a/docs/source/dependencies/dependencies.rst b/docs/source/dependencies/dependencies.rst index 7d468e1..8f58d4e 100644 --- a/docs/source/dependencies/dependencies.rst +++ b/docs/source/dependencies/dependencies.rst @@ -5,7 +5,7 @@ Dependencies .. warning:: - The use of dependencies is now deprecated. See `.thing_connection` and `.ThingServerInterface` for a more intuitive way to access that functionality. + The use of dependencies is now deprecated. See :ref:`thing_connections` and `.ThingServerInterface` for a more intuitive way to access that functionality. LabThings makes use of the powerful "dependency injection" mechanism in FastAPI. You can see the `FastAPI documentation`_ for more information. In brief, FastAPI dependencies are annotated types that instruct FastAPI to supply certain function arguments automatically. This removes the need to set up resources at the start of a function, and ensures everything the function needs is declared and typed clearly. The most common use for dependencies in LabThings is where an action needs to make use of another `.Thing` on the same `.ThingServer`. @@ -14,7 +14,7 @@ Inter-Thing dependencies .. warning:: - These dependencies are deprecated - see `.thing_connection` instead. + These dependencies are deprecated - see :ref:`thing_connections` instead. Simple actions depend only on their input parameters and the `.Thing` on which they are defined. However, it's quite common to need something else, for example accessing another `.Thing` instance on the same LabThings server. There are two important principles to bear in mind here: diff --git a/docs/source/index.rst b/docs/source/index.rst index 81c9301..8d313ab 100644 --- a/docs/source/index.rst +++ b/docs/source/index.rst @@ -11,6 +11,7 @@ Documentation for LabThings-FastAPI tutorial/index.rst examples.rst actions.rst + thing_connections.rst dependencies/dependencies.rst blobs.rst concurrency.rst diff --git a/docs/source/thing_connections.rst b/docs/source/thing_connections.rst new file mode 100644 index 0000000..06df126 --- /dev/null +++ b/docs/source/thing_connections.rst @@ -0,0 +1,89 @@ +.. thing_connections: + +Thing Connections +================= + +It is often desirable for two Things in the same server to be able to communicate. +In order to do this in a nicely typed way that is easy to test and inspect, +LabThings-FastAPI provides `.thing_connection`\ . This allows a `.Thing` +to declare that it depends on another `.Thing` being present, and provides a way for +the server to automatically connect the two when the server is set up. + +Thing connections are set up **after** all the `.Thing` instances are initialised. +This means you should not rely on them during initialisation: if you attempt to +access a connection before it is available, it will raise an exception. The +advantage of making connections after initialisation is that we don't need to +worry about the order in which `.Thing`\ s are created. + +The following example shows the use of a Thing Connection: + +.. code-block:: python + + import labthings_fastapi as lt + + + class ThingA(lt.Thing): + "A class that doesn't do much." + + @lt.action + def say_hello(self) -> str: + "A canonical example function." + return "Hello world." + + + class ThingB(lt.Thing): + "A class that relies on ThingA." + + thing_a: ThingA = lt.thing_connection() + + @lt.action + def say_hello(self) -> str: + "I'm too lazy to say hello, ThingA does it for me." + return self.thing_a.say_hello() + + + server = lt.ThingServer() + server.add_thing("thing_a", ThingA) + server.add_thing("thing_b", ThingB) + + +In this example, ``ThingB.thing_a`` is the simplest form of Thing Connection: it +is type hinted as a `.Thing` subclass, and by default the server will look for the +instance of that class and supply it when the server starts. If there is no +matching `.Thing` or if more than one instance is present, the server will fail +to start with a `.ThingConnectionError`\ . + +It is also possible to use an optional type hint (``ThingA | None``), which +means there will be no error if a matching `.Thing` instance is not found, and +the connection will evaluate to `None`\ . Finally, a `.thing_connection` may be +type hinted as ``Mapping[str, ThingA]`` which permits zero or more instances to +be connected. The mapping keys are the names of the things. + +Configuring Thing Connections +----------------------------- + +A Thing Connection may be given a default value. If this is a string, the server +will look up the `.Thing` by name. If the default is `None` the connection will +evaluate to `None` unless explicitly configured. + +Connections may also be configured when `.Thing`\ s are added to the server: +`.ThingServer.add_thing` takes an argument that allows connections to be made +by name (or set to `None`). Similarly, if you set up your server using a config +file, each entry in the ``things`` list may have a ``thing_connections`` property +that sets up the connections. To repeat the example above with a configuration +file: + +.. code-block:: JSON + + "things": { + "thing_a": "example:ThingA", + "thing_b": { + "class": "example:ThingB", + "thing_connections": { + "thing_a": "thing_a" + } + } + } + +More detail can be found in the description of `.thing_connection` or the +:mod:`.thing_connections` module documentation. From 369faaaf6b28e373c738070d6067bf9748013e8a Mon Sep 17 00:00:00 2001 From: Richard Bowman Date: Tue, 7 Oct 2025 22:41:34 +0100 Subject: [PATCH 29/29] Fix tests for Python < 3.12 Exceptions raised during class definition are wrapped in a RuntimeError prior to Python 3.12, so I use `raises_or_is_called_by` to check the error. It's not ideal that our error gets wrapped in a RuntimeError, but there's relatively little we can do about it. --- tests/test_base_descriptor.py | 8 ++++---- tests/test_property.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_base_descriptor.py b/tests/test_base_descriptor.py index 5562546..91b409f 100644 --- a/tests/test_base_descriptor.py +++ b/tests/test_base_descriptor.py @@ -332,7 +332,7 @@ def test_fieldtyped_definition(name, value_type): def test_fieldtyped_missingtype(): """Check the right error is raised when no type can be found.""" - with pytest.raises(MissingTypeError) as excinfo: + with raises_or_is_caused_by(MissingTypeError) as excinfo: class Example2: field2 = FieldTypedBaseDescriptor() @@ -405,7 +405,7 @@ class Example3: # Rather than roll my own evaluator for forward references, we just # won't support forward references in subscripted types for now. - with pytest.raises(MissingTypeError) as excinfo: + with raises_or_is_caused_by(MissingTypeError) as excinfo: class Example4: field6 = FieldTypedBaseDescriptor["str"]() @@ -418,7 +418,7 @@ class Example4: def test_mismatched_types(): """Check two type hints that don't match raises an error.""" - with pytest.raises(InconsistentTypeError): + with raises_or_is_caused_by(InconsistentTypeError): class Example3: field: int = FieldTypedBaseDescriptor[str]() @@ -445,7 +445,7 @@ def test_stringified_vs_unstringified_mismatch(): If a descriptor is typed using both a subscript and a field annotation, they should match - """ - with pytest.raises(InconsistentTypeError): + with raises_or_is_caused_by(InconsistentTypeError): class Example5: field: "int" = FieldTypedBaseDescriptor[int]() diff --git a/tests/test_property.py b/tests/test_property.py index a401937..a181c9e 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -155,7 +155,7 @@ def test_baseproperty_type_and_model(): `pydantic.RootModel`. """ - with pytest.raises(tp.MissingTypeError): + with raises_or_is_caused_by(tp.MissingTypeError): class Example: prop = tp.BaseProperty()