diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 4eda6517..9c374a04 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -62,7 +62,7 @@ class attribute. Documentation is in strings immediately following the from weakref import WeakSet from fastapi import Body, FastAPI -from pydantic import BaseModel, ConfigDict, RootModel, create_model +from pydantic import BaseModel, ConfigDict, RootModel, ValidationError, create_model from .thing_description import type_to_dataschema from .thing_description._model import ( @@ -71,7 +71,11 @@ class attribute. Documentation is in strings immediately following the PropertyAffordance, PropertyOp, ) -from .utilities import labthings_data, wrap_plain_types_in_rootmodel +from .utilities import ( + LabThingsRootModelWrapper, + labthings_data, + wrap_plain_types_in_rootmodel, +) from .utilities.introspection import return_type from .base_descriptor import ( DescriptorInfoCollection, @@ -859,27 +863,45 @@ def model_instance(self) -> BaseModel: # noqa: DOC201 raise TypeError(msg) return cls(root=value) - def model_to_value(self, value: BaseModel) -> Value: - r"""Convert a model to a value for this property. + def validate(self, value: Any) -> Value: + """Use the validation logic in `self.model`. + + This method should accept anything that `pydantic` can convert to the + right type, and return a correctly-typed value. It also enforces any + constraints that were set on the property. - Even properties with plain types are sometimes converted to or from a - `pydantic.BaseModel` to allow conversion to/from JSON. This is a convenience - method that accepts a model (which should be an instance of ``self.model``\ ) - and unwraps it when necessary to get the plain Python value. + :param value: The new value, in any form acceptable to the property's + model. Usually this means you can use either the correct type, or + the value as loaded from JSON. - :param value: A `.BaseModel` instance to convert. - :return: the value, with `.RootModel` unwrapped so it matches the descriptor's - type. - :raises TypeError: if the supplied value cannot be converted to the right type. + :return: the new value, with the correct type. + + :raises ValidationError: if the supplied value can't be loaded by + the property's model. This is the exception raised by ``model_validate`` + :raises TypeError: if the property has a ``model`` that's inconsistent + with its value type. This should never happen. """ - if isinstance(value, self.value_type): - return value - elif isinstance(value, RootModel): - root = value.root - if isinstance(root, self.value_type): - return root - msg = f"Model {value} isn't {self.value_type} or a RootModel wrapping it." - raise TypeError(msg) + try: + if issubclass(self.model, LabThingsRootModelWrapper): + # If a plain type has been wrapped in a RootModel, use that to validate + # and then set the property to the root value. + model = self.model.model_validate(value) + return model.root + + if issubclass(self.value_type, BaseModel) and self.model is self.value_type: + # If there's no RootModel wrapper, the value was defined in code as a + # Pydantic model. This means `value_type` and `model` should both + # be that same class. + return self.value_type.model_validate(value) + + # This should be unreachable, because `model` is a + # `LabThingsRootModelWrapper` wrapping the value type, or the value type + # should be a BaseModel. + msg = f"Property {self.name} has an inconsistent model. This is " + msg += f"most likely a LabThings bug. {self.model=}, {self.value_type=}" + raise TypeError(msg) + except ValidationError: + raise # This is needed for flake8 to be happy with the docstring class PropertyCollection(DescriptorInfoCollection[Owner, PropertyInfo], Generic[Owner]): @@ -1139,13 +1161,6 @@ def set_without_emit(self, value: Value) -> None: obj = self.owning_object_or_error() self.get_descriptor().set_without_emit(obj, value) - def set_without_emit_from_model(self, value: BaseModel) -> None: - """Set the value from a model instance, unwrapping RootModels as needed. - - :param value: the model to extract the value from. - """ - self.set_without_emit(self.model_to_value(value)) - class SettingCollection(DescriptorInfoCollection[Owner, SettingInfo], Generic[Owner]): """Access to metadata on all the properties of a `.Thing` instance or subclass. diff --git a/src/labthings_fastapi/thing.py b/src/labthings_fastapi/thing.py index 2f245714..71b3b3f0 100644 --- a/src/labthings_fastapi/thing.py +++ b/src/labthings_fastapi/thing.py @@ -188,6 +188,50 @@ def thing_description(request: Request) -> ThingDescription: async def websocket(ws: WebSocket) -> None: await websocket_endpoint(self, ws) + def _read_settings_file(self) -> Mapping[str, Any] | None: + """Read the settings file and return a mapping of saved settings or None. + + This function handles reading the settings from the disk. It is designed + to be called by `load_settings`. Any exceptions caused by file handling or + file corruption are caught and logged as warnings. + + :return: A Mapping of setting name to setting value, or None if no settings + could be read from file. + """ + setting_storage_path = self._thing_server_interface.settings_file_path + thing_name = type(self).__name__ + if not os.path.exists(setting_storage_path): + # If the settings file doesn't exist, we have nothing to do - the settings + # are already initialised to their default values. + return None + + # Load the settings. + try: + with open(setting_storage_path, "r", encoding="utf-8") as file_obj: + settings = json.load(file_obj) + except (FileNotFoundError, PermissionError, JSONDecodeError): + # Note that if the settings file is missing, we should already have + # returned before attempting to load settings. + self.logger.warning( + "Error loading settings for %s from %s, could not load a JSON " + "object. Settings for this Thing will be reset to default.", + thing_name, + setting_storage_path, + ) + return None + + if not isinstance(settings, Mapping): + self.logger.warning( + "Error loading settings for %s from %s. The file does not contain a " + "Mapping", + thing_name, + setting_storage_path, + ) + return None + + # The settings are loaded and are a Mapping. Return them. + return settings + def load_settings(self) -> None: """Load settings from json. @@ -200,32 +244,20 @@ def load_settings(self) -> None: Note that no notifications will be triggered when the settings are set, so if action is needed (e.g. updating hardware with the loaded settings) it should be taken in ``__enter__``. - - :raises TypeError: if the JSON file does not contain a dictionary. This is - handled internally and logged, so the exception doesn't propagate - outside of the function. """ - setting_storage_path = self._thing_server_interface.settings_file_path - thing_name = type(self).__name__ - if not os.path.exists(setting_storage_path): - # If the settings file doesn't exist, we have nothing to do - the settings - # are already initialised to their default values. + settings = self._read_settings_file() + if settings is None: + # Return if no settings can be loaded from file. return # Stop recursion by not allowing settings to be saved as we're reading them. self._disable_saving_settings = True - try: - with open(setting_storage_path, "r", encoding="utf-8") as file_obj: - settings = json.load(file_obj) - if not isinstance(settings, Mapping): - raise TypeError("The settings file must be a JSON object.") for name, value in settings.items(): try: setting = self.settings[name] # Load the key from the JSON file using the setting's model - model = setting.model.model_validate(value) - setting.set_without_emit_from_model(model) + setting.set_without_emit(setting.validate(value)) except ValidationError: self.logger.warning( f"Could not load setting {name} from settings file " @@ -237,19 +269,6 @@ def load_settings(self) -> None: f"An extra key {name} was found in the settings file. " "It will be deleted the next time settings are saved." ) - except ( - FileNotFoundError, - JSONDecodeError, - PermissionError, - TypeError, - ): - # Note that if the settings file is missing, we should already have returned - # before attempting to load settings. - self.logger.warning( - "Error loading settings for %s. " - "Settings for this Thing will be reset to default.", - thing_name, - ) finally: self._disable_saving_settings = False diff --git a/src/labthings_fastapi/utilities/__init__.py b/src/labthings_fastapi/utilities/__init__.py index d1942d1f..13173b4b 100644 --- a/src/labthings_fastapi/utilities/__init__.py +++ b/src/labthings_fastapi/utilities/__init__.py @@ -2,7 +2,7 @@ from __future__ import annotations from collections.abc import Mapping -from typing import Any, Dict, Iterable, TYPE_CHECKING, Optional +from typing import Any, Dict, Generic, Iterable, TYPE_CHECKING, Optional, TypeVar from weakref import WeakSet from pydantic import BaseModel, ConfigDict, Field, RootModel, create_model from pydantic.dataclasses import dataclass @@ -95,6 +95,21 @@ def labthings_data(obj: Thing) -> LabThingsObjectData: return obj.__dict__[LABTHINGS_DICT_KEY] +WrappedT = TypeVar("WrappedT") + + +class LabThingsRootModelWrapper(RootModel[WrappedT], Generic[WrappedT]): + """A RootModel subclass for automatically-wrapped types. + + There are several places where LabThings needs a model, but may only + have a plain Python type. This subclass indicates to LabThings that + a type has been automatically wrapped, and will need to be unwrapped + in order for the value to have the correct type. + + It has no additional functionality. + """ + + def wrap_plain_types_in_rootmodel( model: type, constraints: Mapping[str, Any] | None = None ) -> type[BaseModel]: @@ -131,7 +146,7 @@ def wrap_plain_types_in_rootmodel( return create_model( f"{model!r}", root=(model, Field(**constraints)), - __base__=RootModel, + __base__=LabThingsRootModelWrapper, ) except SchemaError as e: for error in e.errors(): diff --git a/tests/test_property.py b/tests/test_property.py index 4bcd97e3..fe285dbd 100644 --- a/tests/test_property.py +++ b/tests/test_property.py @@ -304,13 +304,34 @@ def prop(self) -> bool: def test_propertyinfo(mocker): """Test out the PropertyInfo class.""" + class MyModel(pydantic.BaseModel): + a: int + b: str + class Example(lt.Thing): intprop: int = lt.property(default=0) """A normal, simple, integer property.""" + positive: int = lt.property(default=0, gt=0) + """A positive integer property.""" + badprop: int = lt.property(default=1) """An integer property that I will break later.""" + tupleprop: tuple[int, str] = lt.property(default=(42, "the answer")) + """A tuple property, to check subscripted generics work.""" + + modelprop: MyModel = lt.property(default_factory=lambda: MyModel(a=1, b="two")) + """A property typed as a model.""" + + rootmodelprop: pydantic.RootModel[int | None] = lt.property( + default_factory=lambda: pydantic.RootModel[int | None](root=None) + ) + """A very verbosely defined optional integer. + + This tests a model that's also a subscripted generic. + """ + # We will break `badprop` by setting its model to something that's # neither the type nor a rootmodel. badprop = Example.badprop @@ -331,11 +352,15 @@ class BadIntModel(pydantic.BaseModel): assert isinstance(model, pydantic.RootModel) assert model.root == 15 - # Check we can unwrap a RootModel correctly - IntModel = example.properties["intprop"].model - assert example.properties["intprop"].model_to_value(IntModel(root=17)) == 17 - with pytest.raises(TypeError): - example.properties["intprop"].model_to_value(BadIntModel(root=17)) + # Check we can validate properly + intprop = example.properties["intprop"] + assert intprop.validate(15) == 15 # integers pass straight through + assert intprop.validate(-15) == -15 + # A RootModel instance ought still to validate + assert intprop.validate(intprop.model(root=42)) == 42 + # A wrong model won't, though. + with pytest.raises(pydantic.ValidationError): + intprop.validate(BadIntModel(root=42)) # Check that a broken `_model` raises the right error # See above for where we manually set badprop._model to something that's @@ -344,6 +369,46 @@ class BadIntModel(pydantic.BaseModel): assert example.badprop == 3 with pytest.raises(TypeError): _ = example.properties["badprop"].model_instance + with pytest.raises(TypeError): + # The value is fine, but the model has been set to an invalid type. + # This error shouldn't be seen in production. + example.properties["badprop"].validate(0) + + # Check validation applies constraints + positive = example.properties["positive"] + assert positive.validate(42) == 42 + with pytest.raises(pydantic.ValidationError): + positive.validate(0) + + # Check validation works for subscripted generics + tupleprop = example.properties["tupleprop"] + assert tupleprop.validate((1, "two")) == (1, "two") + + for val in [0, "str", ("str", 0)]: + with pytest.raises(pydantic.ValidationError): + tupleprop.validate(val) + + # Check validation for a model + modelprop = example.properties["modelprop"] + assert modelprop.validate(MyModel(a=3, b="four")) == MyModel(a=3, b="four") + m = MyModel(a=3, b="four") + assert modelprop.validate(m) is m + assert modelprop.validate({"a": 5, "b": "six"}) == MyModel(a=5, b="six") + for invalid in [{"c": 5}, (4, "f"), None]: + with pytest.raises(pydantic.ValidationError): + modelprop.validate(invalid) + + # Check again for an odd rootmodel + rootmodelprop = example.properties["rootmodelprop"] + m = rootmodelprop.validate(42) + assert isinstance(m, pydantic.RootModel) + assert m.root == 42 + assert m == pydantic.RootModel[int | None](root=42) + assert rootmodelprop.validate(m) is m # RootModel passes through + assert rootmodelprop.validate(None).root is None + for invalid in ["seven", {"root": None}, 14.5, pydantic.RootModel[int](root=0)]: + with pytest.raises(pydantic.ValidationError): + modelprop.validate(invalid) def test_readonly_metadata(): @@ -379,6 +444,7 @@ def _set_funcprop(self, val: int) -> None: example = create_thing_without_server(Example) td = example.thing_description() + assert td.properties is not None # This is mostly for type checking # Check read-write properties are not read-only for name in ["prop", "funcprop"]: diff --git a/tests/test_settings.py b/tests/test_settings.py index afd5bd5a..cc46b7d3 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -40,12 +40,18 @@ def __init__(self, **kwargs: Any) -> None: stringsetting: str = lt.setting(default="foo") "A string setting." - dictsetting: dict = lt.setting(default_factory=lambda: {"a": 1, "b": 2}) - "A dictionary setting." + dictsetting: dict["str", int] = lt.setting(default_factory=lambda: {"a": 1, "b": 2}) + "A dictionary setting. This is a subscripted generic." modelsetting: MyModel = lt.setting(default_factory=lambda: MyModel(a=0, b="string")) "A setting that is a BaseModel." + tuplesetting: tuple[int, int] = lt.setting(default=(1, 2)) + """A tuple setting that specifies component types. + + This is a "subscripted generic" and so has a tendency to break `isinstance` tests. + """ + @lt.setting def floatsetting(self) -> float: """A float setting.""" @@ -115,6 +121,7 @@ def _settings_dict( stringsetting="foo", dictsetting=None, modelsetting=None, + tuplesetting=(1, 2), localonlysetting="Local-only default.", localonly_boolsetting=False, ): @@ -126,12 +133,17 @@ def _settings_dict( dictsetting = {"a": 1, "b": 2} if modelsetting is None: modelsetting = {"a": 0, "b": "string"} + if tuplesetting is None: + tuplesetting = (1, 2) return { "boolsetting": boolsetting, "floatsetting": floatsetting, "stringsetting": stringsetting, "dictsetting": dictsetting, "modelsetting": modelsetting, + # tuples and lists are indestinguishable in JSON, so we convert + # tuplesetting to a list so the comparison will work. + "tuplesetting": list(tuplesetting), "localonlysetting": localonlysetting, "localonly_boolsetting": localonly_boolsetting, } @@ -154,6 +166,7 @@ def test_setting_available(): assert thing.floatsetting == 1.0 assert thing.localonlysetting == "Local-only default." assert thing.dictsetting == {"a": 1, "b": 2} + assert thing.tuplesetting == (1, 2) assert thing.modelsetting == MyModel(a=0, b="string") @@ -245,22 +258,33 @@ def test_settings_dict_internal_update(tempdir): assert not os.path.isfile(setting_file) -def test_settings_load(tempdir): +def test_settings_load(tempdir, caplog): """Check settings can be loaded from disk when added to server""" server = lt.ThingServer({"thing": ThingWithSettings}, settings_folder=tempdir) setting_file = _get_setting_file(server, "thing") del server - setting_json = json.dumps(_settings_dict(floatsetting=3.0, stringsetting="bar")) + setting_json = json.dumps( + _settings_dict( + floatsetting=3.0, + stringsetting="bar", + dictsetting={"c": 3}, + tuplesetting=(77, 22), + ) + ) # Create 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 = lt.ThingServer({"thing": ThingWithSettings}, settings_folder=tempdir) + with caplog.at_level(logging.WARNING): + # Add thing to server and check new settings are loaded + server = lt.ThingServer({"thing": ThingWithSettings}, settings_folder=tempdir) + assert len(caplog.records) == 0 thing = server.things["thing"] assert isinstance(thing, ThingWithSettings) assert not thing.boolsetting assert thing.stringsetting == "bar" assert thing.floatsetting == 3.0 + assert thing.dictsetting == {"c": 3} + assert thing.tuplesetting == (77, 22) def test_load_extra_settings(caplog, tempdir): @@ -307,3 +331,23 @@ def test_try_loading_corrupt_settings(tempdir, caplog): assert len(caplog.records) == 1 assert caplog.records[0].levelname == "WARNING" assert caplog.records[0].name == "labthings_fastapi.things.thing" + + +def test_try_loading_setting_file_without_mapping(tempdir, caplog): + """Load from setting file that isn't a JSON object.""" + # Create the server once, so we can get the settings path + server = lt.ThingServer({"thing": ThingWithSettings}, settings_folder=tempdir) + setting_file = _get_setting_file(server, "thing") + del server + + with open(setting_file, "w", encoding="utf-8") as file_obj: + file_obj.write('["I", "am", "a", "list"]') + # Recreate the server and check for the warning in logs + with caplog.at_level(logging.WARNING): + # Add thing to server + _ = lt.ThingServer({"thing": ThingWithSettings}, settings_folder=tempdir) + assert len(caplog.records) == 1 + assert caplog.records[0].levelname == "WARNING" + assert caplog.records[0].name == "labthings_fastapi.things.thing" + expected_msg = "The file does not contain a Mapping" + assert expected_msg in caplog.records[0].message