Merge pull request #269 from labthings/generic-type-settings

Make loading settings robust to subscripted generics.

Author: rwb27

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.

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
 

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():

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"]: