From 95c83ab6e0c05513683b3b1e8ebc9f03d2da5df9 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Tue, 17 Jun 2025 15:40:09 -0400 Subject: [PATCH 01/17] Exploring making persistant properties instead of settings --- src/labthings_fastapi/decorators/__init__.py | 21 +++++++++++++++++++ src/labthings_fastapi/descriptors/property.py | 6 ++++++ src/labthings_fastapi/server/__init__.py | 5 +++++ 3 files changed, 32 insertions(+) diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index 97cf4eb4..7ed27db6 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -91,6 +91,27 @@ def __get__(self, obj, objtype=None): getter=func, ) +def persistent_thing_property(func: Callable) -> PropertyDescriptor: + """Mark a method of a Thing as a Property + + We replace the function with a `Descriptor` that's a + subclass of `PropertyDescriptor` + + TODO: try https://stackoverflow.com/questions/54413434/type-hinting-with-descriptors + """ + + class PropertyDescriptorSubclass(PropertyDescriptor): + def __get__(self, obj, objtype=None): + return super().__get__(obj, objtype) + + return PropertyDescriptorSubclass( + return_type(func), + readonly=True, + observable=False, + getter=func, + persistent=True, + ) + def fastapi_endpoint(method: HTTPMethod, path: Optional[str] = None, **kwargs): """Add a function to FastAPI as an endpoint""" diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index b8e87dc7..46af0af0 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -3,6 +3,7 @@ """ from __future__ import annotations +import logging from typing import TYPE_CHECKING, Annotated, Any, Callable, Optional from typing_extensions import Self from labthings_fastapi.utilities.introspection import get_summary, get_docstring @@ -38,6 +39,7 @@ def __init__( title: Optional[str] = None, getter: Optional[Callable] = None, setter: Optional[Callable] = None, + persistent: bool = False, ): if getter and initial_value is not None: raise ValueError("getter and an initial value are mutually exclusive.") @@ -52,6 +54,8 @@ def __init__( # The lines below allow _getter and _setter to be specified by subclasses self._setter = setter or getattr(self, "_setter", None) self._getter = getter or getattr(self, "_getter", None) + self.persistent = persistent + self.save_location = None # Try to generate a DataSchema, so that we can raise an error that's easy to # link to the offending PropertyDescriptor type_to_dataschema(self.model) @@ -106,6 +110,8 @@ def __set__(self, obj, value): obj.__dict__[self.name] = value if self._setter: self._setter(obj, value) + if self.persistent: + logging.warning(f'This should save {self.name} to {self.save_location}') self.emit_changed_event(obj, value) def _observers_set(self, obj): diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 3bf8949b..e2de292f 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -18,6 +18,7 @@ from ..thing_description.model import ThingDescription from ..dependencies.thing_server import _thing_servers from ..outputs.blob import BlobDataManager +from ..utilities import class_attributes class ThingServer: @@ -85,6 +86,10 @@ def add_thing(self, thing: Thing, path: str): thing._labthings_thing_settings = ThingSettings( os.path.join(settings_folder, "settings.json") ) + for name, attribute in class_attributes(thing): + if hasattr(attribute, "property_affordance"): + if attribute.persistent: + attribute.save_location = os.path.join(settings_folder, "settings.json") thing.attach_to_server(self, path) @asynccontextmanager From 9680e9b3187675df162da3cec3b2d25deeae5141 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Tue, 17 Jun 2025 18:25:43 -0400 Subject: [PATCH 02/17] Have settings dictionary generated on the fly for saving by a Thing method --- src/labthings_fastapi/decorators/__init__.py | 16 ++++----- src/labthings_fastapi/descriptors/__init__.py | 1 + src/labthings_fastapi/descriptors/property.py | 16 ++++++--- src/labthings_fastapi/server/__init__.py | 7 +--- src/labthings_fastapi/thing.py | 34 ++++++++++++++++--- 5 files changed, 52 insertions(+), 22 deletions(-) diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index 7ed27db6..ed7a2af9 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -37,6 +37,7 @@ from ..descriptors import ( ActionDescriptor, PropertyDescriptor, + SettingDescriptor, EndpointDescriptor, HTTPMethod, ) @@ -91,25 +92,24 @@ def __get__(self, obj, objtype=None): getter=func, ) -def persistent_thing_property(func: Callable) -> PropertyDescriptor: - """Mark a method of a Thing as a Property +def thing_setting(func: Callable) -> SettingDescriptor: + """Mark a method of a Thing as a Setting. - We replace the function with a `Descriptor` that's a - subclass of `PropertyDescriptor` + A setting is a property that persists between runs - TODO: try https://stackoverflow.com/questions/54413434/type-hinting-with-descriptors + We replace the function with a `Descriptor` that's a + subclass of `SettingDescriptor` """ - class PropertyDescriptorSubclass(PropertyDescriptor): + class SettingDescriptorSubclass(SettingDescriptor): def __get__(self, obj, objtype=None): return super().__get__(obj, objtype) - return PropertyDescriptorSubclass( + return SettingDescriptorSubclass( return_type(func), readonly=True, observable=False, getter=func, - persistent=True, ) diff --git a/src/labthings_fastapi/descriptors/__init__.py b/src/labthings_fastapi/descriptors/__init__.py index 5606e383..94a9dd57 100644 --- a/src/labthings_fastapi/descriptors/__init__.py +++ b/src/labthings_fastapi/descriptors/__init__.py @@ -1,4 +1,5 @@ from .action import ActionDescriptor as ActionDescriptor from .property import PropertyDescriptor as PropertyDescriptor +from .property import SettingDescriptor as SettingDescriptor from .endpoint import EndpointDescriptor as EndpointDescriptor from .endpoint import HTTPMethod as HTTPMethod diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index 46af0af0..27f0410a 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -39,7 +39,6 @@ def __init__( title: Optional[str] = None, getter: Optional[Callable] = None, setter: Optional[Callable] = None, - persistent: bool = False, ): if getter and initial_value is not None: raise ValueError("getter and an initial value are mutually exclusive.") @@ -54,7 +53,6 @@ def __init__( # The lines below allow _getter and _setter to be specified by subclasses self._setter = setter or getattr(self, "_setter", None) self._getter = getter or getattr(self, "_getter", None) - self.persistent = persistent self.save_location = None # Try to generate a DataSchema, so that we can raise an error that's easy to # link to the offending PropertyDescriptor @@ -110,8 +108,6 @@ def __set__(self, obj, value): obj.__dict__[self.name] = value if self._setter: self._setter(obj, value) - if self.persistent: - logging.warning(f'This should save {self.name} to {self.save_location}') self.emit_changed_event(obj, value) def _observers_set(self, obj): @@ -227,3 +223,15 @@ def setter(self, func: Callable) -> Self: self._setter = func self.readonly = False return self + + +class SettingDescriptor(PropertyDescriptor): + + @property + def persistent(self): + return True + + def __set__(self, obj, value): + """Set the property's value""" + super().__set__(obj, value) + obj.save_settings() diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index e2de292f..712bd841 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -18,7 +18,6 @@ from ..thing_description.model import ThingDescription from ..dependencies.thing_server import _thing_servers from ..outputs.blob import BlobDataManager -from ..utilities import class_attributes class ThingServer: @@ -86,11 +85,7 @@ def add_thing(self, thing: Thing, path: str): thing._labthings_thing_settings = ThingSettings( os.path.join(settings_folder, "settings.json") ) - for name, attribute in class_attributes(thing): - if hasattr(attribute, "property_affordance"): - if attribute.persistent: - attribute.save_location = os.path.join(settings_folder, "settings.json") - thing.attach_to_server(self, path) + thing.attach_to_server(self, path, os.path.join(settings_folder, "settings.json")) @asynccontextmanager async def lifespan(self, app: FastAPI): diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 06858809..19ac89dc 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -10,11 +10,15 @@ from __future__ import annotations from typing import TYPE_CHECKING, Optional from collections.abc import Mapping +import logging from fastapi.encoders import jsonable_encoder from fastapi import Request from anyio.abc import ObjectSendStream from anyio.from_thread import BlockingPortal from anyio.to_thread import run_sync + +from pydantic import BaseModel + from .descriptors import PropertyDescriptor, ActionDescriptor from .thing_description.model import ThingDescription, NoSecurityScheme from .utilities import class_attributes @@ -35,9 +39,6 @@ class Thing: a particular function - it will correspond to a path on the server, and a Thing Description document. - Your Thing should be a subclass of Thing - note that we don't define `__init__` so - you are free to initialise as you like and don't need to call `super().__init__`. - ## Subclassing Notes * `__init__`: You should accept any arguments you need to configure the Thing @@ -85,10 +86,11 @@ async def __aexit__(self, exc_t, exc_v, exc_tb): if hasattr(self, "__exit__"): return await run_sync(self.__exit__, exc_t, exc_v, exc_tb) - def attach_to_server(self, server: ThingServer, path: str): + def attach_to_server(self, server: ThingServer, path: str, setting_storage_path: str): """Add HTTP handlers to an app for all Interaction Affordances""" self.path = path self.action_manager: ActionManager = server.action_manager + self._setting_storage_path = setting_storage_path for _name, item in class_attributes(self): try: @@ -113,6 +115,30 @@ def thing_description(request: Request) -> ThingDescription: async def websocket(ws: WebSocket): await websocket_endpoint(self, ws) + + _settings: Optional[list[str]] = None + + @property + def settings(self): + if self._settings is None: + self._settings = [] + for name, attribute in class_attributes(self): + if hasattr(attribute, "property_affordance") and hasattr(attribute, "persistent"): + self._settings.append(name) + return self._settings + + _setting_storage_path: Optional[str] = None + + @property + def setting_storage_path(self) -> ThingSettings: + """The storage path for settings. This is set at runtime.""" + return self._setting_storage_path + + def save_settings(self): + if self.settings is not None: + setting_dict = {name: self.__dict__[name] for name in self.settings} + logging.warning(f'This should save {setting_dict} to {self._setting_storage_path}') + _labthings_thing_settings: Optional[ThingSettings] = None @property From c06b097dfaa669a0a7e8a6660895a71e68cc8c46 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 18 Jun 2025 08:35:08 -0400 Subject: [PATCH 03/17] Removing unused settings dict features --- src/labthings_fastapi/server/__init__.py | 15 ---- src/labthings_fastapi/thing.py | 22 +---- src/labthings_fastapi/thing_settings.py | 101 ----------------------- 3 files changed, 4 insertions(+), 134 deletions(-) delete mode 100644 src/labthings_fastapi/thing_settings.py diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 712bd841..d5f0366b 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -13,7 +13,6 @@ object_reference_to_object, ) from ..actions import ActionManager -from ..thing_settings import ThingSettings from ..thing import Thing from ..thing_description.model import ThingDescription from ..dependencies.thing_server import _thing_servers @@ -82,9 +81,6 @@ def add_thing(self, thing: Thing, path: str): # TODO: check for illegal things in `path` - potential security issue. settings_folder = os.path.join(self.settings_folder, path.lstrip("/")) os.makedirs(settings_folder, exist_ok=True) - thing._labthings_thing_settings = ThingSettings( - os.path.join(settings_folder, "settings.json") - ) thing.attach_to_server(self, path, os.path.join(settings_folder, "settings.json")) @asynccontextmanager @@ -117,17 +113,6 @@ async def lifespan(self, app: FastAPI): for name, thing in self.things.items(): # Remove the blocking portal - the event loop is about to stop. thing._labthings_blocking_portal = None - try: - if thing._labthings_thing_settings: - thing._labthings_thing_settings.write_to_file() - except PermissionError: - logging.warning( - f"Could not write {name} settings to disk: permission error." - ) - except FileNotFoundError: - logging.warning( - f"Could not write {name} settings, folder not found" - ) self.blocking_portal = None diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 19ac89dc..b213ad9f 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -25,7 +25,7 @@ from .thing_description import validation from .utilities.introspection import get_summary, get_docstring from .websockets import websocket_endpoint, WebSocket -from .thing_settings import ThingSettings + if TYPE_CHECKING: from .server import ThingServer @@ -130,30 +130,16 @@ def settings(self): _setting_storage_path: Optional[str] = None @property - def setting_storage_path(self) -> ThingSettings: + def setting_storage_path(self) -> str: """The storage path for settings. This is set at runtime.""" return self._setting_storage_path def save_settings(self): + """This is automattically called when a setting is updated with a setter""" if self.settings is not None: - setting_dict = {name: self.__dict__[name] for name in self.settings} + setting_dict = {name: getattr(self, name) for name in self.settings} logging.warning(f'This should save {setting_dict} to {self._setting_storage_path}') - _labthings_thing_settings: Optional[ThingSettings] = None - - @property - def thing_settings(self) -> ThingSettings: - """A dictionary that can be used to persist settings between runs""" - if self._labthings_thing_settings is None: - raise RuntimeError( - "Settings may not be accessed before we are attached to the server." - ) - return self._labthings_thing_settings - - @thing_settings.setter - def thing_settings(self, newsettings: ThingSettings): - self.thing_settings.replace(newsettings) - _labthings_thing_state: Optional[dict] = None @property diff --git a/src/labthings_fastapi/thing_settings.py b/src/labthings_fastapi/thing_settings.py deleted file mode 100644 index 0c04900a..00000000 --- a/src/labthings_fastapi/thing_settings.py +++ /dev/null @@ -1,101 +0,0 @@ -""" -Settings management for things - -ThingSettings is a dictionary-like object that manages settings for one Thing -""" - -from __future__ import annotations -import json -import os -import os.path -from collections.abc import Mapping -from typing import Any, Callable, Optional -from weakref import WeakSet - - -class ReactiveDict(Mapping): - def __init__( - self, - data: Optional[Mapping] = None, - name: Optional[str] = None, - callback: Optional[Callable] = None, - ): - self.name = name if name is not None else "" - self.callbacks: WeakSet[Callable] = WeakSet() - self._data: dict[Any, Any] = {} - if data: - self.replace(data) - if callback: - self.callbacks.add(callback) - - def __getitem__(self, key): - return self._data[key] - - def notify_callbacks(self, path=None): - for c in self.callbacks: - c(self, path) - - def child_callback(self, child: ReactiveDict, path: Any = None): - """Propagate updates from children up to the parent object""" - built_path = child.name - if path: - built_path += f"/{path}" - self.notify_callbacks(path=built_path) - - def __setitem__(self, key, item): - self._data[key] = item - self.notify_callbacks(path=key) - - def __delitem__(self, key): - del self._data[key] - self.notify_callbacks(path=key) - - def __iter__(self): - for k in self._data.keys(): - yield k - - def __len__(self): - return len(self._data) - - def update(self, data: Mapping): - """Update many key-value pairs at once""" - if not isinstance(data, Mapping): - raise ValueError("Config files must be Objects (key-value mappings)") - for k, v in data.items(): - if isinstance(v, Mapping): - self._data[k] = ReactiveDict( - v, name=f"{k}", callback=self.child_callback - ) - else: - self._data[k] = v - self.notify_callbacks() - - def replace(self, data: Mapping): - """Erase all data, then update from the supplied mapping""" - self._data = {} - self.update(data=data) - - @property - def dict(self): - """Return a regular, non-reactive dict of the data""" - out = self._data.copy() - for k, v in self._data.items(): - if isinstance(v, ReactiveDict): - out[k] = v.dict - return out - - -class ThingSettings(ReactiveDict): - def __init__(self, filename: str): - self.filename = filename - if os.path.exists(filename): - with open(filename, "r") as f: - contents = json.load(f) - else: - contents = {} - ReactiveDict.__init__(self, contents, callback=self.write_to_file) - - def write_to_file(self, *args, **kwargs): - """Persist the dictionary to a file""" - with open(self.filename, "w") as f: - json.dump(self.dict, f, indent=4) From c84a0eee0d78c8a5b5fa8e6a7d41558e725eb7c0 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 18 Jun 2025 14:56:49 -0400 Subject: [PATCH 04/17] Give thing settings a way to share a JSON serialisable form version --- src/labthings_fastapi/descriptors/property.py | 19 ++++++++ src/labthings_fastapi/thing.py | 44 +++++++++++++++++-- 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index 27f0410a..7ffbdce0 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -235,3 +235,22 @@ def __set__(self, obj, value): """Set the property's value""" super().__set__(obj, value) obj.save_settings() + + def get_raw(self, obj): + """ + Used to get the raw data for saving settings. Any setting that returns + a non-JSON serialisable class should implement a `raw` flag in its getter + """ + #TODO check if has a raw option rather than use a try + if self._getter: + try: + return self._getter(obj, raw=True) + except TypeError: + return self._getter(obj) + return self.__get__(obj) + + def set_without_emit(self, obj, value): + """Set the property's value, but do not emit. This is called during initial setup""" + obj.__dict__[self.name] = value + if self._setter: + self._setter(obj, value) diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index b213ad9f..48145796 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -11,6 +11,9 @@ from typing import TYPE_CHECKING, Optional from collections.abc import Mapping import logging +import os +import json +from json.decoder import JSONDecodeError from fastapi.encoders import jsonable_encoder from fastapi import Request from anyio.abc import ObjectSendStream @@ -31,6 +34,7 @@ from .server import ThingServer from .actions import ActionManager +_LOGGER = logging.getLogger(__name__) class Thing: """Represents a Thing, as defined by the Web of Things standard. @@ -90,7 +94,7 @@ def attach_to_server(self, server: ThingServer, path: str, setting_storage_path: """Add HTTP handlers to an app for all Interaction Affordances""" self.path = path self.action_manager: ActionManager = server.action_manager - self._setting_storage_path = setting_storage_path + self.load_settings(setting_storage_path) for _name, item in class_attributes(self): try: @@ -134,11 +138,43 @@ def setting_storage_path(self) -> str: """The storage path for settings. This is set at runtime.""" return self._setting_storage_path + def load_settings(self, setting_storage_path): + """Load settings from json. Called when connecting to the server.""" + # Ensure that the settings path isn't set during loading or saving will be triggered + self._setting_storage_path = None + thing_name = type(self).__name__ + if os.path.exists(setting_storage_path): + try: + with open(setting_storage_path, "r", encoding="utf-8") as file_obj: + setting_dict = json.load(file_obj) + setting_attributes = {} + for name, attribute in class_attributes(self): + if hasattr(attribute, "property_affordance") and hasattr(attribute, "persistent"): + setting_attributes[name] = attribute + for key, value in setting_dict.items(): + if key in setting_attributes: + setting_attributes[key].set_without_emit(self, value) + else: + _LOGGER.warning( + "Cannot set %s from persistent storage as %s has no matching setting.", + key, thing_name + ) + except (FileNotFoundError, JSONDecodeError, PermissionError): + _LOGGER.warning("Error loading settings for %s", thing_name) + self._setting_storage_path = setting_storage_path + + def save_settings(self): - """This is automattically called when a setting is updated with a setter""" + """Save settings to JSON. This is called when a setting is updated with a setter""" if self.settings is not None: - setting_dict = {name: getattr(self, name) for name in self.settings} - logging.warning(f'This should save {setting_dict} to {self._setting_storage_path}') + setting_dict = {} + for name, attribute in class_attributes(self): + if hasattr(attribute, "property_affordance") and hasattr(attribute, "persistent"): + setting_dict[name] = attribute.get_raw(self) + # Dumpy to string before writing so if this fails the file isn't overwritten + setting_json = json.dumps(setting_dict, indent=4) + with open(self._setting_storage_path, "w", encoding="utf-8") as file_obj: + file_obj.write(setting_json) _labthings_thing_state: Optional[dict] = None From 7749ce366158d54abad6523fef165d8d1ee2dcea Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 18 Jun 2025 14:58:26 -0400 Subject: [PATCH 05/17] ruff formatting --- src/labthings_fastapi/decorators/__init__.py | 1 + src/labthings_fastapi/descriptors/property.py | 3 +-- src/labthings_fastapi/server/__init__.py | 4 +++- src/labthings_fastapi/thing.py | 24 ++++++++++++------- 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index ed7a2af9..6bfaf6c6 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -92,6 +92,7 @@ def __get__(self, obj, objtype=None): getter=func, ) + def thing_setting(func: Callable) -> SettingDescriptor: """Mark a method of a Thing as a Setting. diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index 7ffbdce0..c2809dcd 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -226,7 +226,6 @@ def setter(self, func: Callable) -> Self: class SettingDescriptor(PropertyDescriptor): - @property def persistent(self): return True @@ -241,7 +240,7 @@ def get_raw(self, obj): Used to get the raw data for saving settings. Any setting that returns a non-JSON serialisable class should implement a `raw` flag in its getter """ - #TODO check if has a raw option rather than use a try + # TODO check if has a raw option rather than use a try if self._getter: try: return self._getter(obj, raw=True) diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index d5f0366b..425ac101 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -81,7 +81,9 @@ def add_thing(self, thing: Thing, path: str): # TODO: check for illegal things in `path` - potential security issue. settings_folder = os.path.join(self.settings_folder, path.lstrip("/")) os.makedirs(settings_folder, exist_ok=True) - thing.attach_to_server(self, path, os.path.join(settings_folder, "settings.json")) + thing.attach_to_server( + self, path, os.path.join(settings_folder, "settings.json") + ) @asynccontextmanager async def lifespan(self, app: FastAPI): diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 48145796..2a2e23b1 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -36,6 +36,7 @@ _LOGGER = logging.getLogger(__name__) + class Thing: """Represents a Thing, as defined by the Web of Things standard. @@ -90,7 +91,9 @@ async def __aexit__(self, exc_t, exc_v, exc_tb): if hasattr(self, "__exit__"): return await run_sync(self.__exit__, exc_t, exc_v, exc_tb) - def attach_to_server(self, server: ThingServer, path: str, setting_storage_path: str): + def attach_to_server( + self, server: ThingServer, path: str, setting_storage_path: str + ): """Add HTTP handlers to an app for all Interaction Affordances""" self.path = path self.action_manager: ActionManager = server.action_manager @@ -119,7 +122,6 @@ def thing_description(request: Request) -> ThingDescription: async def websocket(ws: WebSocket): await websocket_endpoint(self, ws) - _settings: Optional[list[str]] = None @property @@ -127,7 +129,9 @@ def settings(self): if self._settings is None: self._settings = [] for name, attribute in class_attributes(self): - if hasattr(attribute, "property_affordance") and hasattr(attribute, "persistent"): + if hasattr(attribute, "property_affordance") and hasattr( + attribute, "persistent" + ): self._settings.append(name) return self._settings @@ -149,27 +153,31 @@ def load_settings(self, setting_storage_path): setting_dict = json.load(file_obj) setting_attributes = {} for name, attribute in class_attributes(self): - if hasattr(attribute, "property_affordance") and hasattr(attribute, "persistent"): + if hasattr(attribute, "property_affordance") and hasattr( + attribute, "persistent" + ): setting_attributes[name] = attribute for key, value in setting_dict.items(): if key in setting_attributes: - setting_attributes[key].set_without_emit(self, value) + setting_attributes[key].set_without_emit(self, value) else: _LOGGER.warning( "Cannot set %s from persistent storage as %s has no matching setting.", - key, thing_name + key, + thing_name, ) except (FileNotFoundError, JSONDecodeError, PermissionError): _LOGGER.warning("Error loading settings for %s", thing_name) self._setting_storage_path = setting_storage_path - def save_settings(self): """Save settings to JSON. This is called when a setting is updated with a setter""" if self.settings is not None: setting_dict = {} for name, attribute in class_attributes(self): - if hasattr(attribute, "property_affordance") and hasattr(attribute, "persistent"): + if hasattr(attribute, "property_affordance") and hasattr( + attribute, "persistent" + ): setting_dict[name] = attribute.get_raw(self) # Dumpy to string before writing so if this fails the file isn't overwritten setting_json = json.dumps(setting_dict, indent=4) From bdc3352cdde38d9717a7600c75003a6bcf7ca287 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Wed, 18 Jun 2025 17:52:42 -0400 Subject: [PATCH 06/17] Removing ad hoc test methods like get_raw and instead handling BaseModels gracefully --- src/labthings_fastapi/descriptors/property.py | 13 ------- src/labthings_fastapi/thing.py | 34 ++++++++----------- 2 files changed, 14 insertions(+), 33 deletions(-) diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index c2809dcd..e37e330c 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -235,19 +235,6 @@ def __set__(self, obj, value): super().__set__(obj, value) obj.save_settings() - def get_raw(self, obj): - """ - Used to get the raw data for saving settings. Any setting that returns - a non-JSON serialisable class should implement a `raw` flag in its getter - """ - # TODO check if has a raw option rather than use a try - if self._getter: - try: - return self._getter(obj, raw=True) - except TypeError: - return self._getter(obj) - return self.__get__(obj) - def set_without_emit(self, obj, value): """Set the property's value, but do not emit. This is called during initial setup""" obj.__dict__[self.name] = value diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 2a2e23b1..7d550ee3 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -126,13 +126,13 @@ async def websocket(ws: WebSocket): @property def settings(self): - if self._settings is None: - self._settings = [] - for name, attribute in class_attributes(self): - if hasattr(attribute, "property_affordance") and hasattr( - attribute, "persistent" - ): - self._settings.append(name) + if self._settings is not None: + return self._settings + + self._settings = {} + for name, attr in class_attributes(self): + if hasattr(attr, "property_affordance") and hasattr(attr, "persistent"): + self._settings[name] = attr return self._settings _setting_storage_path: Optional[str] = None @@ -151,15 +151,9 @@ def load_settings(self, setting_storage_path): try: with open(setting_storage_path, "r", encoding="utf-8") as file_obj: setting_dict = json.load(file_obj) - setting_attributes = {} - for name, attribute in class_attributes(self): - if hasattr(attribute, "property_affordance") and hasattr( - attribute, "persistent" - ): - setting_attributes[name] = attribute for key, value in setting_dict.items(): - if key in setting_attributes: - setting_attributes[key].set_without_emit(self, value) + if key in self.settings: + self.settings[key].set_without_emit(self, value) else: _LOGGER.warning( "Cannot set %s from persistent storage as %s has no matching setting.", @@ -174,11 +168,11 @@ def save_settings(self): """Save settings to JSON. This is called when a setting is updated with a setter""" if self.settings is not None: setting_dict = {} - for name, attribute in class_attributes(self): - if hasattr(attribute, "property_affordance") and hasattr( - attribute, "persistent" - ): - setting_dict[name] = attribute.get_raw(self) + for name in self.settings.keys(): + value = getattr(self, name) + if isinstance(value, BaseModel): + value = value.model_dump() + setting_dict[name] = value # Dumpy to string before writing so if this fails the file isn't overwritten setting_json = json.dumps(setting_dict, indent=4) with open(self._setting_storage_path, "w", encoding="utf-8") as file_obj: From 40c9ef4a0ad2cccc2b019c99329798aade694348 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 19 Jun 2025 08:29:52 -0400 Subject: [PATCH 07/17] Remove vestigial code --- src/labthings_fastapi/decorators/__init__.py | 20 ++++++------------- src/labthings_fastapi/descriptors/property.py | 2 -- src/labthings_fastapi/server/__init__.py | 1 - 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index 6bfaf6c6..c7901ef5 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -75,17 +75,13 @@ def thing_action(func: Optional[Callable] = None, **kwargs): def thing_property(func: Callable) -> PropertyDescriptor: """Mark a method of a Thing as a Property - We replace the function with a `Descriptor` that's a - subclass of `PropertyDescriptor` + Replace the function with a `Descriptor` that's a + `PropertyDescriptor` TODO: try https://stackoverflow.com/questions/54413434/type-hinting-with-descriptors """ - class PropertyDescriptorSubclass(PropertyDescriptor): - def __get__(self, obj, objtype=None): - return super().__get__(obj, objtype) - - return PropertyDescriptorSubclass( + return PropertyDescriptor( return_type(func), readonly=True, observable=False, @@ -98,15 +94,11 @@ def thing_setting(func: Callable) -> SettingDescriptor: A setting is a property that persists between runs - We replace the function with a `Descriptor` that's a - subclass of `SettingDescriptor` + Replace the function with a `Descriptor` that's a + `SettingDescriptor` """ - class SettingDescriptorSubclass(SettingDescriptor): - def __get__(self, obj, objtype=None): - return super().__get__(obj, objtype) - - return SettingDescriptorSubclass( + return SettingDescriptor( return_type(func), readonly=True, observable=False, diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index e37e330c..02079572 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -3,7 +3,6 @@ """ from __future__ import annotations -import logging from typing import TYPE_CHECKING, Annotated, Any, Callable, Optional from typing_extensions import Self from labthings_fastapi.utilities.introspection import get_summary, get_docstring @@ -53,7 +52,6 @@ def __init__( # The lines below allow _getter and _setter to be specified by subclasses self._setter = setter or getattr(self, "_setter", None) self._getter = getter or getattr(self, "_getter", None) - self.save_location = None # Try to generate a DataSchema, so that we can raise an error that's easy to # link to the offending PropertyDescriptor type_to_dataschema(self.model) diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 425ac101..12fb3e99 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -1,5 +1,4 @@ from __future__ import annotations -import logging from typing import Optional, Sequence, TypeVar import os.path from fastapi import FastAPI, Request From 125092f2c2c300d4b30370c6c1232a35db4aa4be Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Thu, 19 Jun 2025 15:18:50 -0400 Subject: [PATCH 08/17] Change the names for setting/property descriptors, add some docstrings --- src/labthings_fastapi/client/in_server.py | 8 ++-- src/labthings_fastapi/decorators/__init__.py | 37 ++++++++++++------- src/labthings_fastapi/descriptors/__init__.py | 4 +- src/labthings_fastapi/descriptors/property.py | 18 ++++++--- .../example_things/__init__.py | 6 +-- src/labthings_fastapi/thing.py | 8 ++-- 6 files changed, 49 insertions(+), 32 deletions(-) diff --git a/src/labthings_fastapi/client/in_server.py b/src/labthings_fastapi/client/in_server.py index 533c34e2..32508ba9 100644 --- a/src/labthings_fastapi/client/in_server.py +++ b/src/labthings_fastapi/client/in_server.py @@ -16,7 +16,7 @@ from pydantic import BaseModel from labthings_fastapi.descriptors.action import ActionDescriptor -from labthings_fastapi.descriptors.property import PropertyDescriptor +from labthings_fastapi.descriptors.property import ThingProperty from labthings_fastapi.utilities import attributes from . import PropertyClientDescriptor from ..thing import Thing @@ -123,7 +123,7 @@ def action_method(self, **kwargs): def add_property( - attrs: dict[str, Any], property_name: str, property: PropertyDescriptor + attrs: dict[str, Any], property_name: str, property: ThingProperty ) -> None: """Add a property to a DirectThingClient subclass""" attrs[property_name] = property_descriptor( @@ -131,7 +131,7 @@ def add_property( property.model, description=property.description, writeable=not property.readonly, - readable=True, # TODO: make this configurable in PropertyDescriptor + readable=True, # TODO: make this configurable in ThingProperty ) @@ -163,7 +163,7 @@ def init_proxy(self, request: Request, **dependencies: Mapping[str, Any]): } dependencies: list[inspect.Parameter] = [] for name, item in attributes(thing_class): - if isinstance(item, PropertyDescriptor): + if isinstance(item, ThingProperty): # TODO: What about properties that don't use descriptors? Fall back to http? add_property(client_attrs, name, item) elif isinstance(item, ActionDescriptor): diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index c7901ef5..1955001a 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -36,8 +36,8 @@ from typing import Optional, Callable from ..descriptors import ( ActionDescriptor, - PropertyDescriptor, - SettingDescriptor, + ThingProperty, + ThingSetting, EndpointDescriptor, HTTPMethod, ) @@ -72,16 +72,18 @@ def thing_action(func: Optional[Callable] = None, **kwargs): return partial(mark_thing_action, **kwargs) -def thing_property(func: Callable) -> PropertyDescriptor: +def thing_property(func: Callable) -> ThingProperty: """Mark a method of a Thing as a Property - Replace the function with a `Descriptor` that's a - `PropertyDescriptor` - - TODO: try https://stackoverflow.com/questions/54413434/type-hinting-with-descriptors + As properties are accessed over the HTTP API they need to be JSON serialisable + only return standard python types, or Pydantic BaseModels """ - return PropertyDescriptor( + # Replace the function with a `Descriptor` that's a `ThingProperty` + + # TODO: try https://stackoverflow.com/questions/54413434/type-hinting-with-descriptors + + return ThingProperty( return_type(func), readonly=True, observable=False, @@ -89,16 +91,23 @@ def thing_property(func: Callable) -> PropertyDescriptor: ) -def thing_setting(func: Callable) -> SettingDescriptor: +def thing_setting(func: Callable) -> ThingSetting: """Mark a method of a Thing as a Setting. - A setting is a property that persists between runs + When creating a Setting you must always create a setter as it is used to load + from disk. - Replace the function with a `Descriptor` that's a - `SettingDescriptor` - """ + A setting is a property that persists between runs. + + As settings are accessed over the HTTP API and saved to disk they need to be + JSON serialisable only return standard python types, or Pydantic BaseModels. - return SettingDescriptor( + If the type is a pydantic BaseModel, then the setter must also be able to accept + the dictionary representation of this BaseModel as this is what will be used to + set the Setting when loading from disk on starting the server. + """ + # Replace the function with a `Descriptor` that's a `ThingSetting` + return ThingSetting( return_type(func), readonly=True, observable=False, diff --git a/src/labthings_fastapi/descriptors/__init__.py b/src/labthings_fastapi/descriptors/__init__.py index 94a9dd57..901b14ad 100644 --- a/src/labthings_fastapi/descriptors/__init__.py +++ b/src/labthings_fastapi/descriptors/__init__.py @@ -1,5 +1,5 @@ from .action import ActionDescriptor as ActionDescriptor -from .property import PropertyDescriptor as PropertyDescriptor -from .property import SettingDescriptor as SettingDescriptor +from .property import ThingProperty as ThingProperty +from .property import ThingSetting as ThingSetting from .endpoint import EndpointDescriptor as EndpointDescriptor from .endpoint import HTTPMethod as HTTPMethod diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index 02079572..792e2620 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -18,10 +18,10 @@ from ..thing import Thing -class PropertyDescriptor: +class ThingProperty: """A property that can be accessed via the HTTP API - By default, a PropertyDescriptor is "dumb", i.e. it acts just like + By default, a ThingProperty is "dumb", i.e. it acts just like a normal variable. """ @@ -53,7 +53,7 @@ def __init__( self._setter = setter or getattr(self, "_setter", None) self._getter = getter or getattr(self, "_getter", None) # Try to generate a DataSchema, so that we can raise an error that's easy to - # link to the offending PropertyDescriptor + # link to the offending ThingProperty type_to_dataschema(self.model) def __set_name__(self, owner, name: str): @@ -214,7 +214,7 @@ def getter(self, func: Callable) -> Self: def setter(self, func: Callable) -> Self: """Decorator to set the property's value - PropertyDescriptors are variabes - so they will return the value they hold + ThingPropertys are variabes - so they will return the value they hold when they are accessed. However, they can run code when they are set: this decorator sets a function as that code. """ @@ -223,7 +223,15 @@ def setter(self, func: Callable) -> Self: return self -class SettingDescriptor(PropertyDescriptor): +class ThingSetting(ThingProperty): + """A setting can be accessed via the HTTP API and is persistent between sessions + + A ThingSetting is a ThingProperty with extra functionality for triggering + a Thing to save its settings, and for setting a property without emitting an event so + that the setting can be set from disk before the server is fully started. + + The setting otherwise acts just like a normal variable. + """ @property def persistent(self): return True diff --git a/src/labthings_fastapi/example_things/__init__.py b/src/labthings_fastapi/example_things/__init__.py index 6cfaca03..61d4d7ff 100644 --- a/src/labthings_fastapi/example_things/__init__.py +++ b/src/labthings_fastapi/example_things/__init__.py @@ -6,7 +6,7 @@ from typing import Optional, Annotated from labthings_fastapi.thing import Thing from labthings_fastapi.decorators import thing_action, thing_property -from labthings_fastapi.descriptors import PropertyDescriptor +from labthings_fastapi.descriptors import ThingProperty from pydantic import Field @@ -73,11 +73,11 @@ def slowly_increase_counter(self, increments: int = 60, delay: float = 1): time.sleep(delay) self.increment_counter() - counter = PropertyDescriptor( + counter = ThingProperty( model=int, initial_value=0, readonly=True, description="A pointless counter" ) - foo = PropertyDescriptor( + foo = ThingProperty( model=str, initial_value="Example", description="A pointless string for demo purposes.", diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 7d550ee3..da2d93cf 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -22,7 +22,7 @@ from pydantic import BaseModel -from .descriptors import PropertyDescriptor, ActionDescriptor +from .descriptors import ThingProperty, ThingSetting, ActionDescriptor from .thing_description.model import ThingDescription, NoSecurityScheme from .utilities import class_attributes from .thing_description import validation @@ -56,7 +56,7 @@ class Thing: code that will close down your hardware. It's equivalent to a `finally:` block. * Properties and Actions are defined using decorators: the `@thing_action` decorator declares a method to be an action, which will run when it's triggered, and the - `@thing_property` decorator (or `PropertyDescriptor` descriptor) does the same for + `@thing_property` decorator (or `ThingProperty` descriptor) does the same for a property. See the documentation on those functions for more detail. * `title` will be used in various places as the human-readable name of your Thing, so it makes sense to set this in a subclass. @@ -131,7 +131,7 @@ def settings(self): self._settings = {} for name, attr in class_attributes(self): - if hasattr(attr, "property_affordance") and hasattr(attr, "persistent"): + if isinstance(attr, ThingSetting): self._settings[name] = attr return self._settings @@ -264,7 +264,7 @@ def thing_description_dict( def observe_property(self, property_name: str, stream: ObjectSendStream): """Register a stream to receive property change notifications""" prop = getattr(self.__class__, property_name) - if not isinstance(prop, PropertyDescriptor): + if not isinstance(prop, ThingProperty): raise KeyError(f"{property_name} is not a LabThings Property") prop._observers_set(self).add(stream) From bb214f1382eea1f2d856c6060f98c325d226480d Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Sat, 21 Jun 2025 05:41:20 -0400 Subject: [PATCH 09/17] Update PropertyDescriptor to ThingProperty in all examples and tests --- docs/source/quickstart/counter.py | 4 ++-- examples/counter.py | 4 ++-- examples/demo_thing_server.py | 6 +++--- examples/opencv_camera_server.py | 4 ++-- examples/picamera2_camera_server.py | 20 +++++++++---------- src/labthings_fastapi/descriptors/property.py | 1 + tests/test_action_cancel.py | 4 ++-- tests/test_action_manager.py | 4 ++-- tests/test_properties.py | 12 +++++------ tests/test_thing_lifecycle.py | 4 ++-- 10 files changed, 31 insertions(+), 32 deletions(-) diff --git a/docs/source/quickstart/counter.py b/docs/source/quickstart/counter.py index 66d2f509..9416ad72 100644 --- a/docs/source/quickstart/counter.py +++ b/docs/source/quickstart/counter.py @@ -1,7 +1,7 @@ import time from labthings_fastapi.thing import Thing from labthings_fastapi.decorators import thing_action -from labthings_fastapi.descriptors import PropertyDescriptor +from labthings_fastapi.descriptors import ThingProperty class TestThing(Thing): @@ -24,7 +24,7 @@ def slowly_increase_counter(self) -> None: time.sleep(1) self.increment_counter() - counter = PropertyDescriptor( + counter = ThingProperty( model=int, initial_value=0, readonly=True, description="A pointless counter" ) diff --git a/examples/counter.py b/examples/counter.py index 268a9c76..14158aae 100644 --- a/examples/counter.py +++ b/examples/counter.py @@ -1,7 +1,7 @@ import time from labthings_fastapi.thing import Thing from labthings_fastapi.decorators import thing_action -from labthings_fastapi.descriptors import PropertyDescriptor +from labthings_fastapi.descriptors import ThingProperty from labthings_fastapi.server import ThingServer @@ -25,7 +25,7 @@ def slowly_increase_counter(self) -> None: time.sleep(1) self.increment_counter() - counter = PropertyDescriptor( + counter = ThingProperty( model=int, initial_value=0, readonly=True, description="A pointless counter" ) diff --git a/examples/demo_thing_server.py b/examples/demo_thing_server.py index 143e1fed..5c27f141 100644 --- a/examples/demo_thing_server.py +++ b/examples/demo_thing_server.py @@ -4,7 +4,7 @@ from labthings_fastapi.thing import Thing from labthings_fastapi.decorators import thing_action from labthings_fastapi.server import ThingServer -from labthings_fastapi.descriptors import PropertyDescriptor +from labthings_fastapi.descriptors import ThingProperty from pydantic import Field from fastapi.responses import HTMLResponse @@ -60,11 +60,11 @@ def slowly_increase_counter(self): time.sleep(1) self.increment_counter() - counter = PropertyDescriptor( + counter = ThingProperty( model=int, initial_value=0, readonly=True, description="A pointless counter" ) - foo = PropertyDescriptor( + foo = ThingProperty( model=str, initial_value="Example", description="A pointless string for demo purposes.", diff --git a/examples/opencv_camera_server.py b/examples/opencv_camera_server.py index dc904898..01bfa4f1 100644 --- a/examples/opencv_camera_server.py +++ b/examples/opencv_camera_server.py @@ -3,7 +3,7 @@ from fastapi import FastAPI from fastapi.responses import HTMLResponse, StreamingResponse -from labthings_fastapi.descriptors.property import PropertyDescriptor +from labthings_fastapi.descriptors.property import ThingProperty from labthings_fastapi.thing import Thing from labthings_fastapi.decorators import thing_action, thing_property from labthings_fastapi.server import ThingServer @@ -279,7 +279,7 @@ def exposure(self, value): with self._cap_lock: self._cap.set(cv.CAP_PROP_EXPOSURE, value) - last_frame_index = PropertyDescriptor(int, initial_value=-1) + last_frame_index = ThingProperty(int, initial_value=-1) mjpeg_stream = MJPEGStreamDescriptor(ringbuffer_size=10) diff --git a/examples/picamera2_camera_server.py b/examples/picamera2_camera_server.py index 7208b1d2..4347f130 100644 --- a/examples/picamera2_camera_server.py +++ b/examples/picamera2_camera_server.py @@ -4,7 +4,7 @@ from pydantic import BaseModel, BeforeValidator -from labthings_fastapi.descriptors.property import PropertyDescriptor +from labthings_fastapi.descriptors.property import ThingProperty from labthings_fastapi.thing import Thing from labthings_fastapi.decorators import thing_action, thing_property from labthings_fastapi.server import ThingServer @@ -24,14 +24,12 @@ logging.basicConfig(level=logging.INFO) -class PicameraControl(PropertyDescriptor): +class PicameraControl(ThingProperty): def __init__( self, control_name: str, model: type = float, description: Optional[str] = None ): """A property descriptor controlling a picamera control""" - PropertyDescriptor.__init__( - self, model, observable=False, description=description - ) + ThingProperty.__init__(self, model, observable=False, description=description) self.control_name = control_name self._getter @@ -84,20 +82,20 @@ def __init__(self, device_index: int = 0): self.device_index = device_index self.camera_configs: dict[str, dict] = {} - stream_resolution = PropertyDescriptor( + stream_resolution = ThingProperty( tuple[int, int], initial_value=(1640, 1232), description="Resolution to use for the MJPEG stream", ) - image_resolution = PropertyDescriptor( + image_resolution = ThingProperty( tuple[int, int], initial_value=(3280, 2464), description="Resolution to use for still images (by default)", ) - mjpeg_bitrate = PropertyDescriptor( + mjpeg_bitrate = ThingProperty( int, initial_value=0, description="Bitrate for MJPEG stream (best left at 0)" ) - stream_active = PropertyDescriptor( + stream_active = ThingProperty( bool, initial_value=False, description="Whether the MJPEG stream is active", @@ -116,7 +114,7 @@ def __init__(self, device_index: int = 0): exposure_time = PicameraControl( "ExposureTime", int, description="The exposure time in microseconds" ) - sensor_modes = PropertyDescriptor(list[SensorMode], readonly=True) + sensor_modes = ThingProperty(list[SensorMode], readonly=True) def __enter__(self): self._picamera = picamera2.Picamera2(camera_num=self.device_index) @@ -219,7 +217,7 @@ def exposure(self) -> float: def exposure(self, value): raise NotImplementedError() - last_frame_index = PropertyDescriptor(int, initial_value=-1) + last_frame_index = ThingProperty(int, initial_value=-1) mjpeg_stream = MJPEGStreamDescriptor(ringbuffer_size=10) diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index 792e2620..9e8d0928 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -232,6 +232,7 @@ class ThingSetting(ThingProperty): The setting otherwise acts just like a normal variable. """ + @property def persistent(self): return True diff --git a/tests/test_action_cancel.py b/tests/test_action_cancel.py index 50422eae..9a9f6e75 100644 --- a/tests/test_action_cancel.py +++ b/tests/test_action_cancel.py @@ -8,12 +8,12 @@ from temp_client import poll_task, task_href from labthings_fastapi.thing import Thing from labthings_fastapi.decorators import thing_action -from labthings_fastapi.descriptors import PropertyDescriptor +from labthings_fastapi.descriptors import ThingProperty from labthings_fastapi.dependencies.invocation import CancelHook class ThingOne(Thing): - counter = PropertyDescriptor(int, 0, observable=False) + counter = ThingProperty(int, 0, observable=False) @thing_action def count_slowly(self, cancel: CancelHook, n: int = 10): diff --git a/tests/test_action_manager.py b/tests/test_action_manager.py index fe5c7c60..9c17dbc6 100644 --- a/tests/test_action_manager.py +++ b/tests/test_action_manager.py @@ -6,7 +6,7 @@ import time from labthings_fastapi.thing import Thing from labthings_fastapi.decorators import thing_action -from labthings_fastapi.descriptors import PropertyDescriptor +from labthings_fastapi.descriptors import ThingProperty class TestThing(Thing): @@ -15,7 +15,7 @@ def increment_counter(self): """Increment the counter""" self.counter += 1 - counter = PropertyDescriptor( + counter = ThingProperty( model=int, initial_value=0, readonly=True, description="A pointless counter" ) diff --git a/tests/test_properties.py b/tests/test_properties.py index f9c84f60..157e99a0 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -1,4 +1,4 @@ -from labthings_fastapi.descriptors import PropertyDescriptor +from labthings_fastapi.descriptors import ThingProperty from labthings_fastapi.decorators import thing_property, thing_action from labthings_fastapi.thing import Thing from fastapi.testclient import TestClient @@ -9,8 +9,8 @@ class TestThing(Thing): - boolprop = PropertyDescriptor(bool, False, description="A boolean property") - stringprop = PropertyDescriptor(str, "foo", description="A string property") + boolprop = ThingProperty(bool, False, description="A boolean property") + stringprop = ThingProperty(str, "foo", description="A string property") _undoc = None @@ -44,7 +44,7 @@ def toggle_boolprop_from_thread(self): def test_instantiation_with_type(): - prop = PropertyDescriptor(bool, False) + prop = ThingProperty(bool, False) assert issubclass(prop.model, BaseModel) @@ -53,7 +53,7 @@ class MyModel(BaseModel): a: int = 1 b: float = 2.0 - prop = PropertyDescriptor(MyModel, MyModel()) + prop = ThingProperty(MyModel, MyModel()) assert prop.model is MyModel @@ -65,7 +65,7 @@ def test_property_get_and_set(): assert after_value.json() == test_str -def test_propertydescriptor(): +def test_ThingProperty(): with TestClient(server.app) as client: r = client.get("/thing/boolprop") assert r.json() is False diff --git a/tests/test_thing_lifecycle.py b/tests/test_thing_lifecycle.py index 4c848e3e..1172e3da 100644 --- a/tests/test_thing_lifecycle.py +++ b/tests/test_thing_lifecycle.py @@ -1,11 +1,11 @@ -from labthings_fastapi.descriptors import PropertyDescriptor +from labthings_fastapi.descriptors import ThingProperty from labthings_fastapi.thing import Thing from fastapi.testclient import TestClient from labthings_fastapi.server import ThingServer class TestThing(Thing): - alive = PropertyDescriptor(bool, False, description="Is the thing alive?") + alive = ThingProperty(bool, False, description="Is the thing alive?") def __enter__(self): print("setting up TestThing from __enter__") From 941b52031c576e97e7ef1a10e1a4a576c0bf4626 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Sat, 21 Jun 2025 05:51:36 -0400 Subject: [PATCH 10/17] Fix typehint in setting storage path --- src/labthings_fastapi/thing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index da2d93cf..6421b1f3 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -138,7 +138,7 @@ def settings(self): _setting_storage_path: Optional[str] = None @property - def setting_storage_path(self) -> str: + def setting_storage_path(self) -> Optional[str]: """The storage path for settings. This is set at runtime.""" return self._setting_storage_path From e4afc1b194c6f07823e6ac413df7ea36867a2277 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Sat, 21 Jun 2025 06:32:36 -0400 Subject: [PATCH 11/17] Update docstrings for Thing Setting changes --- src/labthings_fastapi/decorators/__init__.py | 24 +++++---- src/labthings_fastapi/descriptors/property.py | 14 +++--- src/labthings_fastapi/thing.py | 50 +++++++++++++------ 3 files changed, 57 insertions(+), 31 deletions(-) diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index 1955001a..bc686752 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -73,16 +73,17 @@ def thing_action(func: Optional[Callable] = None, **kwargs): def thing_property(func: Callable) -> ThingProperty: - """Mark a method of a Thing as a Property + """Mark a method of a Thing as a LabThings Property + + This should be used as a decorator with a getter and a setter + just like a standard python property decorator. If extra functionality + is not required in the decorator, then using the ThingProperty class + directly may allow for clearer code As properties are accessed over the HTTP API they need to be JSON serialisable only return standard python types, or Pydantic BaseModels """ - # Replace the function with a `Descriptor` that's a `ThingProperty` - - # TODO: try https://stackoverflow.com/questions/54413434/type-hinting-with-descriptors - return ThingProperty( return_type(func), readonly=True, @@ -92,13 +93,18 @@ def thing_property(func: Callable) -> ThingProperty: def thing_setting(func: Callable) -> ThingSetting: - """Mark a method of a Thing as a Setting. - - When creating a Setting you must always create a setter as it is used to load - from disk. + """Mark a method of a Thing as a LabThings Setting. A setting is a property that persists between runs. + This should be used as a decorator with a getter and a setter + just like a standard python property decorator. If extra functionality + is not required in the decorator, then using the ThingSetting class + directly may allow for clearer code + + When creating a Setting using this decorator you must always create a setter + as it is used to load the value from disk. + As settings are accessed over the HTTP API and saved to disk they need to be JSON serialisable only return standard python types, or Pydantic BaseModels. diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index 9e8d0928..d6414dbc 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -227,23 +227,23 @@ class ThingSetting(ThingProperty): """A setting can be accessed via the HTTP API and is persistent between sessions A ThingSetting is a ThingProperty with extra functionality for triggering - a Thing to save its settings, and for setting a property without emitting an event so - that the setting can be set from disk before the server is fully started. + a Thing to save its settings. The setting otherwise acts just like a normal variable. """ - @property - def persistent(self): - return True - def __set__(self, obj, value): """Set the property's value""" super().__set__(obj, value) obj.save_settings() def set_without_emit(self, obj, value): - """Set the property's value, but do not emit. This is called during initial setup""" + """Set the property's value, but do not emit event to notify the server + + This function is not expected to be used externally. It is called during + initial setup so that the setting can be set from disk before the server + is fully started. + """ obj.__dict__[self.name] = value if self._setter: self._setter(obj, value) diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 6421b1f3..e9cc7a02 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -94,7 +94,18 @@ async def __aexit__(self, exc_t, exc_v, exc_tb): def attach_to_server( self, server: ThingServer, path: str, setting_storage_path: str ): - """Add HTTP handlers to an app for all Interaction Affordances""" + """Attatch this thing to the server. + + Things need to be attached to a server before use to function correctly. + + :param server: The server to attach this Thing to + :param settings_storage_path: The path on disk to save the any Thing Settings + to. This should be the path to a json file. If it does not exist it will be + created. + + Wc3 Web Of Things explanation: + This will add HTTP handlers to an app for all Interaction Affordances + """ self.path = path self.action_manager: ActionManager = server.action_manager self.load_settings(setting_storage_path) @@ -122,28 +133,37 @@ def thing_description(request: Request) -> ThingDescription: async def websocket(ws: WebSocket): await websocket_endpoint(self, ws) - _settings: Optional[list[str]] = None + # A private variable to hold the list of settings so it doesn't need to be + # iterated through each time it is read + _settings_store: Optional[dict[str, ThingSetting]] = None @property - def settings(self): - if self._settings is not None: - return self._settings + def _settings(self) -> Optional[dict[str, ThingSetting]]: + """A private property that returns a dict of all settings for this Thing + + Each dict key is the name of the setting, the corresponding value is the + ThingSetting class (a descriptor). This can be used to directly get the + descriptor so that the value can be set without emitting signals, such + as on startup. + """ + if self._settings_store is not None: + return self._settings_store - self._settings = {} + self._settings_store = {} for name, attr in class_attributes(self): if isinstance(attr, ThingSetting): - self._settings[name] = attr - return self._settings + self._settings_store[name] = attr + return self._settings_store _setting_storage_path: Optional[str] = None @property def setting_storage_path(self) -> Optional[str]: - """The storage path for settings. This is set at runtime.""" + """The storage path for settings. This is set as the Thing is added to a server""" return self._setting_storage_path def load_settings(self, setting_storage_path): - """Load settings from json. Called when connecting to the server.""" + """Load settings from json. This is run when the Thing is added to a server""" # Ensure that the settings path isn't set during loading or saving will be triggered self._setting_storage_path = None thing_name = type(self).__name__ @@ -152,8 +172,8 @@ def load_settings(self, setting_storage_path): with open(setting_storage_path, "r", encoding="utf-8") as file_obj: setting_dict = json.load(file_obj) for key, value in setting_dict.items(): - if key in self.settings: - self.settings[key].set_without_emit(self, value) + if key in self._settings: + self._settings[key].set_without_emit(self, value) else: _LOGGER.warning( "Cannot set %s from persistent storage as %s has no matching setting.", @@ -165,10 +185,10 @@ def load_settings(self, setting_storage_path): self._setting_storage_path = setting_storage_path def save_settings(self): - """Save settings to JSON. This is called when a setting is updated with a setter""" - if self.settings is not None: + """Save settings to JSON. This is called whenever a setting is updated""" + if self._settings is not None: setting_dict = {} - for name in self.settings.keys(): + for name in self._settings.keys(): value = getattr(self, name) if isinstance(value, BaseModel): value = value.model_dump() From 5af35ed8be2aa372986ab888d40f8a51d3eeebf5 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Sun, 22 Jun 2025 13:06:32 +0100 Subject: [PATCH 12/17] Add unit tests and docstrings. Also add pathcheck when addign a thing --- pyproject.toml | 8 + src/labthings_fastapi/descriptors/action.py | 21 ++- src/labthings_fastapi/descriptors/property.py | 18 +- src/labthings_fastapi/exceptions.py | 11 ++ src/labthings_fastapi/server/__init__.py | 24 ++- tests/test_properties.py | 22 ++- tests/test_settings.py | 155 ++++++++++++++++++ tests/test_thing.py | 18 ++ 8 files changed, 262 insertions(+), 15 deletions(-) create mode 100644 src/labthings_fastapi/exceptions.py create mode 100644 tests/test_settings.py diff --git a/pyproject.toml b/pyproject.toml index b9be87eb..7ccbb713 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -51,6 +51,14 @@ artifacts = ["src/*.json"] [tool.hatch.build.targets.wheel] artifacts = ["src/*.json"] +[tool.pytest.ini_options] +addopts = [ + "--cov=labthings_fastapi", + "--cov-report=term", + "--cov-report=xml:coverage.xml", + "--cov-report=html:htmlcov", +] + [tool.ruff] target-version = "py310" diff --git a/src/labthings_fastapi/descriptors/action.py b/src/labthings_fastapi/descriptors/action.py index de29dfef..3ddde574 100644 --- a/src/labthings_fastapi/descriptors/action.py +++ b/src/labthings_fastapi/descriptors/action.py @@ -6,8 +6,11 @@ from functools import partial import inspect from typing import TYPE_CHECKING, Annotated, Any, Callable, Optional, Literal, overload +from weakref import WeakSet + from fastapi import Body, FastAPI, Request, BackgroundTasks from pydantic import create_model + from ..actions import InvocationModel from ..dependencies.invocation import CancelHook, InvocationID from ..dependencies.action_manager import ActionManagerContextDep @@ -23,9 +26,8 @@ from ..outputs.blob import BlobIOContextDep from ..thing_description import type_to_dataschema from ..thing_description.model import ActionAffordance, ActionOp, Form, Union - -from weakref import WeakSet from ..utilities import labthings_data, get_blocking_portal +from ..exceptions import NotConnectedToServerError if TYPE_CHECKING: from ..thing import Thing @@ -129,12 +131,23 @@ def _observers_set(self, obj): def emit_changed_event(self, obj, status): """Notify subscribers that the action status has changed - NB this function **must** be run from a thread, not the event loop. + This function is run from within the `Invocation` thread that + is created when an action is called. It must be run from this thread + as it is communicating with the event loop via an `asyncio` blocking + portal. + + :raises NotConnectedToServerError: if the Thing calling the action is not + connected to a server with a running event loop. """ try: runner = get_blocking_portal(obj) if not runner: - raise RuntimeError("Can't emit without a blocking portal") + thing_name = obj.__class__.__name__ + msg = ( + f"Cannot emit action changed event. Is {thing_name} connected to " + "a running server?" + ) + raise NotConnectedToServerError(msg) runner.start_task_soon( self.emit_changed_event_async, obj, diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index d6414dbc..74bd7302 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -4,14 +4,17 @@ from __future__ import annotations from typing import TYPE_CHECKING, Annotated, Any, Callable, Optional +from weakref import WeakSet + from typing_extensions import Self -from labthings_fastapi.utilities.introspection import get_summary, get_docstring from pydantic import BaseModel, RootModel from fastapi import Body, FastAPI -from weakref import WeakSet + from ..utilities import labthings_data, wrap_plain_types_in_rootmodel +from ..utilities.introspection import get_summary, get_docstring from ..thing_description.model import PropertyAffordance, Form, DataSchema, PropertyOp from ..thing_description import type_to_dataschema +from ..exceptions import NotConnectedToServerError if TYPE_CHECKING: @@ -118,11 +121,18 @@ def _observers_set(self, obj): def emit_changed_event(self, obj: Thing, value: Any) -> None: """Notify subscribers that the property has changed - NB this function **must** be run from a thread, not the event loop. + This function is run when properties are upadated. It must be run from + within a thread. This could be the `Invocation` thread of a running action, or + the property should be updated over via a client/http. It must be run from a + thread as it is communicating with the event loop via an `asyncio` blocking + portal. + + :raises NotConnectedToServerError: if the Thing that is calling the property + update is not connected to a server with a running event loop. """ runner = obj._labthings_blocking_portal if not runner: - raise RuntimeError("Can't emit without a blocking portal") + raise NotConnectedToServerError("Can't emit without a blocking portal") runner.start_task_soon( self.emit_changed_event_async, obj, diff --git a/src/labthings_fastapi/exceptions.py b/src/labthings_fastapi/exceptions.py new file mode 100644 index 00000000..26072db1 --- /dev/null +++ b/src/labthings_fastapi/exceptions.py @@ -0,0 +1,11 @@ +"""A submodule for custom LabThings-FastAPI Exceptions""" + + +class NotConnectedToServerError(RuntimeError): + """The Thing is not connected to a server + + This exception is called if a ThingAction is called or + is a ThingProperty is updated on a Thing that is not + connected to a ThingServer. A server connection is needed + to manage asynchronous behaviour. + """ diff --git a/src/labthings_fastapi/server/__init__.py b/src/labthings_fastapi/server/__init__.py index 12fb3e99..9fdfbe52 100644 --- a/src/labthings_fastapi/server/__init__.py +++ b/src/labthings_fastapi/server/__init__.py @@ -1,6 +1,8 @@ from __future__ import annotations from typing import Optional, Sequence, TypeVar import os.path +import re + from fastapi import FastAPI, Request from fastapi.middleware.cors import CORSMiddleware from anyio.from_thread import BlockingPortal @@ -17,6 +19,11 @@ from ..dependencies.thing_server import _thing_servers from ..outputs.blob import BlobDataManager +# A path should be made up of names separated by / as a path separator. +# Each name should be made of alphanumeric characters, hyphen, or underscore. +# This regex enforces a trailing / +PATH_REGEX = re.compile(r"^/([a-zA-Z0-9\-_]+\/)+$") + class ThingServer: def __init__(self, settings_folder: Optional[str] = None): @@ -71,13 +78,26 @@ def thing_by_class(self, cls: type[ThingInstance]) -> ThingInstance: ) def add_thing(self, thing: Thing, path: str): - """Add a thing to the server""" + """Add a thing to the server + + :param thing: The thing to add to the server. + :param path: the relative path to access the thing on the server. Must only + contain alphanumeric characters, hyphens, or underscores. + """ + # Ensure leading and trailing / if not path.endswith("/"): path += "/" + if not path.startswith("/"): + path = "/" + path + if PATH_REGEX.match(path) is None: + msg = ( + f"{path} contains unsafe characters. Use only alphanumeric " + "characters, hyphens and underscores" + ) + raise ValueError(msg) if path in self._things: raise KeyError(f"{path} has already been added to this thing server.") self._things[path] = thing - # TODO: check for illegal things in `path` - potential security issue. settings_folder = os.path.join(self.settings_folder, path.lstrip("/")) os.makedirs(settings_folder, exist_ok=True) thing.attach_to_server( diff --git a/tests/test_properties.py b/tests/test_properties.py index 157e99a0..7109a53a 100644 --- a/tests/test_properties.py +++ b/tests/test_properties.py @@ -1,11 +1,14 @@ +from threading import Thread + +from pytest import raises +from pydantic import BaseModel +from fastapi.testclient import TestClient + from labthings_fastapi.descriptors import ThingProperty from labthings_fastapi.decorators import thing_property, thing_action from labthings_fastapi.thing import Thing -from fastapi.testclient import TestClient from labthings_fastapi.server import ThingServer -from threading import Thread -from pytest import raises -from pydantic import BaseModel +from labthings_fastapi.exceptions import NotConnectedToServerError class TestThing(Thing): @@ -44,6 +47,12 @@ def toggle_boolprop_from_thread(self): def test_instantiation_with_type(): + """ + Check the internal model (data type) of the ThingSetting descriptor is a BaseModel + + To send the data over HTTP LabThings-FastAPI uses Pydantic models to describe data + types. + """ prop = ThingProperty(bool, False) assert issubclass(prop.model, BaseModel) @@ -117,8 +126,11 @@ def test_setting_from_thread(): def test_setting_without_event_loop(): + """Test that an exception is raised if updating a ThingProperty + without connecting the Thing to a running server with an event loop. + """ # This test may need to change, if we change the intended behaviour # Currently it should never be necessary to change properties from the # main thread, so we raise an error if you try to do so - with raises(RuntimeError): + with raises(NotConnectedToServerError): thing.boolprop = False # Can't call it until the event loop's running diff --git a/tests/test_settings.py b/tests/test_settings.py new file mode 100644 index 00000000..6843b1c0 --- /dev/null +++ b/tests/test_settings.py @@ -0,0 +1,155 @@ +from threading import Thread +import tempfile +import json +import pytest +import os +import logging + +from fastapi.testclient import TestClient + +from labthings_fastapi.descriptors import ThingSetting +from labthings_fastapi.decorators import thing_setting, thing_action +from labthings_fastapi.thing import Thing +from labthings_fastapi.server import ThingServer + + +class TestThing(Thing): + boolsetting = ThingSetting(bool, False, description="A boolean setting") + stringsetting = ThingSetting(str, "foo", description="A string setting") + + _float = 1.0 + + @thing_setting + def floatsetting(self) -> float: + return self._float + + @floatsetting.setter + def floatsetting(self, value: float): + self._float = value + + @thing_action + def toggle_boolsetting(self): + self.boolsetting = not self.boolsetting + + @thing_action + def toggle_boolsetting_from_thread(self): + t = Thread(target=self.toggle_boolsetting) + t.start() + + +def _get_setting_file(server, thingpath): + path = os.path.join(server.settings_folder, thingpath.lstrip("/"), "settings.json") + return os.path.normpath(path) + + +def _settings_dict(boolsetting=False, floatsetting=1.0, stringsetting="foo"): + """Return the expected settings dictionary + + Parameters can be updated from default values + """ + return { + "boolsetting": boolsetting, + "floatsetting": floatsetting, + "stringsetting": stringsetting, + } + + +@pytest.fixture +def thing(): + return TestThing() + + +@pytest.fixture +def server(): + with tempfile.TemporaryDirectory() as tempdir: + # Yield server rather than return so that the temp directory isn't cleaned up + # until after the test is run + yield ThingServer(settings_folder=tempdir) + + +def test_setting_available(thing): + """Check default settings are available before connecting to server""" + assert not thing.boolsetting + assert thing.stringsetting == "foo" + assert thing.floatsetting == 1.0 + + +def test_settings_save(thing, server): + """Check updated settings are saved to disk""" + setting_file = _get_setting_file(server, "/thing") + server.add_thing(thing, "/thing") + # No setting file created when first added + assert not os.path.isfile(setting_file) + with TestClient(server.app) as client: + r = client.put("/thing/floatsetting", json=2.0) + assert r.status_code == 201 + r = client.get("/thing/floatsetting") + assert r.json() == 2.0 + assert os.path.isfile(setting_file) + with open(setting_file, "r", encoding="utf-8") as file_obj: + # Check settings on file match expected dictionary + assert json.load(file_obj) == _settings_dict(floatsetting=2.0) + + +def test_settings_load(thing, server): + """Check settings can be loaded from disk when added to server""" + setting_file = _get_setting_file(server, "/thing") + setting_json = json.dumps(_settings_dict(floatsetting=3.0, stringsetting="bar")) + # Create setting file + os.makedirs(os.path.dirname(setting_file)) + with open(setting_file, "w", encoding="utf-8") as file_obj: + file_obj.write(setting_json) + # Add thing to server and check new settings are loaded + server.add_thing(thing, "/thing") + assert not thing.boolsetting + assert thing.stringsetting == "bar" + assert thing.floatsetting == 3.0 + + +def test_load_extra_settings(thing, server, caplog): + """Load from setting file. Extra setting in file should create a warning.""" + setting_file = _get_setting_file(server, "/thing") + setting_dict = _settings_dict(floatsetting=3.0, stringsetting="bar") + setting_dict["extra_setting"] = 33.33 + setting_json = json.dumps(setting_dict) + # Create setting file + os.makedirs(os.path.dirname(setting_file)) + with open(setting_file, "w", encoding="utf-8") as file_obj: + file_obj.write(setting_json) + + with caplog.at_level(logging.WARNING): + # Add thing to server + server.add_thing(thing, "/thing") + assert len(caplog.records) == 1 + assert caplog.records[0].levelname == "WARNING" + assert caplog.records[0].name == "labthings_fastapi.thing" + + # Check other settings are loaded as expected + assert not thing.boolsetting + assert thing.stringsetting == "bar" + assert thing.floatsetting == 3.0 + + +def test_try_loading_corrupt_settings(thing, server, caplog): + """Load from setting file. Extra setting in file should create a warning.""" + setting_file = _get_setting_file(server, "/thing") + setting_dict = _settings_dict(floatsetting=3.0, stringsetting="bar") + setting_json = json.dumps(setting_dict) + # Cut the start off the json to so it can't be decoded. + setting_json = setting_json[3:] + # Create setting file + os.makedirs(os.path.dirname(setting_file)) + with open(setting_file, "w", encoding="utf-8") as file_obj: + file_obj.write(setting_json) + + with caplog.at_level(logging.WARNING): + # Add thing to server + server.add_thing(thing, "/thing") + assert len(caplog.records) == 1 + assert caplog.records[0].levelname == "WARNING" + assert caplog.records[0].name == "labthings_fastapi.thing" + + # Check default settings are loaded + assert not thing.boolsetting + assert thing.stringsetting == "foo" + assert thing.floatsetting == 1.0 diff --git a/tests/test_thing.py b/tests/test_thing.py index b247432d..ab31fb96 100644 --- a/tests/test_thing.py +++ b/tests/test_thing.py @@ -1,7 +1,25 @@ +import pytest from labthings_fastapi.example_things import MyThing +from labthings_fastapi.server import ThingServer def test_td_validates(): """This will raise an exception if it doesn't validate OK""" thing = MyThing() assert thing.validate_thing_description() is None + + +def test_add_thing(): + """Check that thing can be added to the server""" + thing = MyThing() + server = ThingServer() + server.add_thing(thing, "/thing") + + +def test_add_naughty_thing(): + """Check that a thing trying to access server resources + using .. is not allowed""" + thing = MyThing() + server = ThingServer() + with pytest.raises(ValueError): + server.add_thing(thing, "/../../../../bin") From 06eaf4ad1b8a6b53037b9ec263af0441b7db8f58 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Sun, 22 Jun 2025 14:29:32 +0100 Subject: [PATCH 13/17] Add small section on Properties vs Settings in the docs --- docs/source/lt_core_concepts.rst | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/docs/source/lt_core_concepts.rst b/docs/source/lt_core_concepts.rst index 128898ba..7501d9b3 100644 --- a/docs/source/lt_core_concepts.rst +++ b/docs/source/lt_core_concepts.rst @@ -10,21 +10,26 @@ At its core LabThings FastAPI is a server-based framework. To use LabThings Fast The server API is accessed over an HTTP requests, allowing client code (see below) to be written in any language that can send an HTTP request. -Client Code ------------ - -Clients or client code (Not to be confused with a :class:`.ThingClient`, see below) is the terminology used to describe any software that uses HTTP requests to access the LabThing Server. Clients can be written in any language that supports an HTTP request. However, LabThings FastAPI provides additional functionality that makes writing client code in Python easier. - Everything is a Thing --------------------- -As described in :doc:`wot_core_concepts`, a Thing represents a piece of hardware or software. LabThings-FastAPI automatically generates a `Thing Description`_ to describe each Thing. Each function offered by the Thing is either a Property, Action, or Event. These are termed "interaction affordances" in WoT_ terminology. +As described in :doc:`wot_core_concepts`, a Thing represents a piece of hardware or software. LabThings-FastAPI automatically generates a `Thing Description`_ to describe each Thing. Each function offered by the Thing is either a Property or Action (LabThings-FastAPI does not yet support Events). These are termed "interaction affordances" in WoT_ terminology. Code on the LabThings FastAPI Server is composed of Things, however these can call generic Python functions/classes. The entire HTTP API served by the server is defined by :class:`.Thing` objects. As such the full API is composed of the actions and properties (and perhaps eventually events) defined in each Thing. _`Thing Description`: wot_core_concepts#thing _`WoT`: wot_core_concepts +Properties vs Settings +---------------------- + +A Thing in LabThings-FastAPI can have Settings as well as Properties. "Setting" is LabThings-FastAPI terminology for a "Property" with a value that persists after the server is restarted. All Settings are Properties, and -- except for persisting after a server restart -- Settings are identical to any other Properties. + +Client Code +----------- + +Clients or client code (Not to be confused with a :class:`.ThingClient`, see below) is the terminology used to describe any software that uses HTTP requests to access the LabThing Server. Clients can be written in any language that supports an HTTP request. However, LabThings FastAPI provides additional functionality that makes writing client code in Python easier. + ThingClients ------------ From 919db288cba98d64086d4b293ab33f657173f9b9 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Sun, 22 Jun 2025 15:03:34 +0100 Subject: [PATCH 14/17] Add clarification that settings must be explicitly set to save Add unit test, that is clear that docstrings should be updated if the behaviour changes --- src/labthings_fastapi/decorators/__init__.py | 6 +++ src/labthings_fastapi/descriptors/property.py | 8 +++- tests/test_settings.py | 41 ++++++++++++++++++- 3 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index bc686752..583b8eca 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -111,6 +111,12 @@ def thing_setting(func: Callable) -> ThingSetting: If the type is a pydantic BaseModel, then the setter must also be able to accept the dictionary representation of this BaseModel as this is what will be used to set the Setting when loading from disk on starting the server. + + Note: If a setting updated, rather than explicitly set this will not trigger saving. + For example: if a Thing has a setting called `dictsetting` holding the dictionary + `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving + but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never + called. """ # Replace the function with a `Descriptor` that's a `ThingSetting` return ThingSetting( diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index 74bd7302..e0e0c741 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -224,7 +224,7 @@ def getter(self, func: Callable) -> Self: def setter(self, func: Callable) -> Self: """Decorator to set the property's value - ThingPropertys are variabes - so they will return the value they hold + ThingPropertys are variables - so they will return the value they hold when they are accessed. However, they can run code when they are set: this decorator sets a function as that code. """ @@ -239,6 +239,12 @@ class ThingSetting(ThingProperty): A ThingSetting is a ThingProperty with extra functionality for triggering a Thing to save its settings. + Note: If a setting updated, rather than explicitly set this will not trigger saving. + For example: if a Thing has a setting called `dictsetting` holding the dictionary + `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving + but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never + called. + The setting otherwise acts just like a normal variable. """ diff --git a/tests/test_settings.py b/tests/test_settings.py index 6843b1c0..495f579b 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -16,6 +16,9 @@ class TestThing(Thing): boolsetting = ThingSetting(bool, False, description="A boolean setting") stringsetting = ThingSetting(str, "foo", description="A string setting") + dictsetting = ThingSetting( + dict, {"a": 1, "b": 2}, description="A dictionary setting" + ) _float = 1.0 @@ -42,15 +45,20 @@ def _get_setting_file(server, thingpath): return os.path.normpath(path) -def _settings_dict(boolsetting=False, floatsetting=1.0, stringsetting="foo"): +def _settings_dict( + boolsetting=False, floatsetting=1.0, stringsetting="foo", dictsetting=None +): """Return the expected settings dictionary Parameters can be updated from default values """ + if dictsetting is None: + dictsetting = {"a": 1, "b": 2} return { "boolsetting": boolsetting, "floatsetting": floatsetting, "stringsetting": stringsetting, + "dictsetting": dictsetting, } @@ -91,6 +99,37 @@ def test_settings_save(thing, server): assert json.load(file_obj) == _settings_dict(floatsetting=2.0) +def test_settings_dict_save(thing, server): + """Check settings are saved if the dict is updated in full""" + setting_file = _get_setting_file(server, "/thing") + server.add_thing(thing, "/thing") + # No setting file created when first added + assert not os.path.isfile(setting_file) + with TestClient(server.app): + thing.dictsetting = {"c": 3} + assert os.path.isfile(setting_file) + with open(setting_file, "r", encoding="utf-8") as file_obj: + # Check settings on file match expected dictionary + assert json.load(file_obj) == _settings_dict(dictsetting={"c": 3}) + + +def test_settings_dict_internal_update(thing, server): + """Confirm settings are not saved if the internal value of a dictionary is updated + + This behaviour is not ideal, but it is documented. If the behaviour is updated + then the documentation should be updated and this test removed + """ + setting_file = _get_setting_file(server, "/thing") + server.add_thing(thing, "/thing") + # No setting file created when first added + assert not os.path.isfile(setting_file) + with TestClient(server.app): + thing.dictsetting["a"] = 4 + # As only an internal member of the dictornary was set, the saving was not + # triggered. + assert not os.path.isfile(setting_file) + + def test_settings_load(thing, server): """Check settings can be loaded from disk when added to server""" setting_file = _get_setting_file(server, "/thing") From e5fbd4f390b54f75544a627f72a823d3142bd5ee Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Sun, 22 Jun 2025 15:16:49 +0100 Subject: [PATCH 15/17] Update error message for changed event on property --- src/labthings_fastapi/descriptors/property.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index e0e0c741..127a211d 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -132,7 +132,12 @@ def emit_changed_event(self, obj: Thing, value: Any) -> None: """ runner = obj._labthings_blocking_portal if not runner: - raise NotConnectedToServerError("Can't emit without a blocking portal") + thing_name = obj.__class__.__name__ + msg = ( + f"Cannot emit property updated changed event. Is {thing_name} " + "connected to a running server?" + ) + raise NotConnectedToServerError(msg) runner.start_task_soon( self.emit_changed_event_async, obj, From 01af5bbf453f3d7c8807f2a202e9701e10755f19 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Tue, 24 Jun 2025 22:04:07 +0100 Subject: [PATCH 16/17] Removed outdated TODO Co-authored-by: Richard Bowman --- src/labthings_fastapi/client/in_server.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/labthings_fastapi/client/in_server.py b/src/labthings_fastapi/client/in_server.py index 32508ba9..b6459be1 100644 --- a/src/labthings_fastapi/client/in_server.py +++ b/src/labthings_fastapi/client/in_server.py @@ -164,7 +164,6 @@ def init_proxy(self, request: Request, **dependencies: Mapping[str, Any]): dependencies: list[inspect.Parameter] = [] for name, item in attributes(thing_class): if isinstance(item, ThingProperty): - # TODO: What about properties that don't use descriptors? Fall back to http? add_property(client_attrs, name, item) elif isinstance(item, ActionDescriptor): if actions is None or name in actions: From 2f880a4593ac7cd81c595793d4f23cc56e5d3891 Mon Sep 17 00:00:00 2001 From: Julian Stirling Date: Tue, 24 Jun 2025 22:15:04 +0100 Subject: [PATCH 17/17] Apply suggestions from code review Co-authored-by: Richard Bowman --- pyproject.toml | 1 + src/labthings_fastapi/decorators/__init__.py | 4 ++-- src/labthings_fastapi/descriptors/property.py | 4 ++-- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 7ccbb713..97e23577 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -57,6 +57,7 @@ addopts = [ "--cov-report=term", "--cov-report=xml:coverage.xml", "--cov-report=html:htmlcov", + "--cov-report=lcov", ] [tool.ruff] diff --git a/src/labthings_fastapi/decorators/__init__.py b/src/labthings_fastapi/decorators/__init__.py index 583b8eca..8ae57991 100644 --- a/src/labthings_fastapi/decorators/__init__.py +++ b/src/labthings_fastapi/decorators/__init__.py @@ -100,7 +100,7 @@ def thing_setting(func: Callable) -> ThingSetting: This should be used as a decorator with a getter and a setter just like a standard python property decorator. If extra functionality is not required in the decorator, then using the ThingSetting class - directly may allow for clearer code + directly may allow for clearer code where the property works like a normal variable. When creating a Setting using this decorator you must always create a setter as it is used to load the value from disk. @@ -112,7 +112,7 @@ def thing_setting(func: Callable) -> ThingSetting: the dictionary representation of this BaseModel as this is what will be used to set the Setting when loading from disk on starting the server. - Note: If a setting updated, rather than explicitly set this will not trigger saving. + Note: If a setting is mutated rather than set, this will not trigger saving. For example: if a Thing has a setting called `dictsetting` holding the dictionary `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never diff --git a/src/labthings_fastapi/descriptors/property.py b/src/labthings_fastapi/descriptors/property.py index 127a211d..f51a31cf 100644 --- a/src/labthings_fastapi/descriptors/property.py +++ b/src/labthings_fastapi/descriptors/property.py @@ -229,7 +229,7 @@ def getter(self, func: Callable) -> Self: def setter(self, func: Callable) -> Self: """Decorator to set the property's value - ThingPropertys are variables - so they will return the value they hold + ``ThingProperty`` descriptors return the value they hold when they are accessed. However, they can run code when they are set: this decorator sets a function as that code. """ @@ -244,7 +244,7 @@ class ThingSetting(ThingProperty): A ThingSetting is a ThingProperty with extra functionality for triggering a Thing to save its settings. - Note: If a setting updated, rather than explicitly set this will not trigger saving. + Note: If a setting is mutated rather than assigned to, this will not trigger saving. For example: if a Thing has a setting called `dictsetting` holding the dictionary `{"a": 1, "b": 2}` then `self.dictsetting = {"a": 2, "b": 2}` would trigger saving but `self.dictsetting[a] = 2` would not, as the setter for `dictsetting` is never