Validate properties on assignment

src/labthings_fastapi/properties.py

@@ -608,7 +608,8 @@
         :param value: the new value for the property.
         :param emit_changed_event: whether to emit a changed event.
         """
-        obj.__dict__[self.name] = value
+        property_info = self.descriptor_info(obj)
+        obj.__dict__[self.name] = property_info.validate(value)
         if emit_changed_event:
             self.emit_changed_event(obj, value)
 
@@ -696,11 +697,11 @@
         self._fget = fget
         self._type = return_type(self._fget)
         if self._type is None:
             msg = (
                 f"{fget} does not have a valid type. "
                 "Return type annotations are required for property getters."
             )
             raise MissingTypeError(msg)
         self._fset: Callable[[Owner, Value], None] | None = None
         self.readonly: bool = True
 
@@ -723,10 +724,10 @@
         :param fget: The new getter function.
         :return: this descriptor (i.e. ``self``). This allows use as a decorator.
         """
         self._fget = fget
         self._type = return_type(self._fget)
         self.__doc__ = fget.__doc__
         return self
 
     def setter(self, fset: Callable[[Owner, Value], None]) -> Self:
         r"""Set the setter function of the property.
@@ -798,7 +799,7 @@
             # Don't return the descriptor if it's named differently.
             # see typing notes in docstring.
             return fset  # type: ignore[return-value]
         return self
 
     def instance_get(self, obj: Owner) -> Value:
         """Get the value of the property.
@@ -811,13 +812,19 @@
     def __set__(self, obj: Owner, value: Value) -> None:
         """Set the value of the property.
 
+        This will validate the value against the property's model, and an error
+        will be raised if the value is not valid.
+
         :param obj: the `.Thing` on which the attribute is accessed.
         :param value: the value of the property.
 
         :raises ReadOnlyPropertyError: if the property cannot be set.
         """
         if self.fset is None:
             raise ReadOnlyPropertyError(f"Property {self.name} of {obj} has no setter.")
+        property_info = self.descriptor_info(obj)
+        value = property_info.validate(value)
+
         self.fset(obj, value)
 
 
@@ -1044,7 +1051,7 @@
 
         :raises NotImplementedError: this method should be implemented in subclasses.
         """
         raise NotImplementedError("This method should be implemented in subclasses.")
 
     def descriptor_info(self, owner: Owner | None = None) -> SettingInfo[Owner, Value]:
         r"""Return an object that allows access to this descriptor's metadata.

tests/test_properties.py

@@ -378,21 +378,44 @@ def test_setting_without_event_loop():
 
 
 @pytest.mark.parametrize("prop_info", CONSTRAINED_PROPS)
-def test_constrained_properties(prop_info):
-    """Test that constraints on property values generate correct models."""
+def test_constrained_properties(prop_info, mocker):
+    """Test that constraints on property values generate correct models.
+
+    This also tests the `validate` method and checks validation happens
+    on assignment to the property in Python. Further checks over http
+    are made in later tests.
+    """
     prop = prop_info.prop
     assert prop.value_type is prop_info.value_type
     m = prop.model
     assert issubclass(m, RootModel)
+    mock_thing = mocker.Mock(spec=PropertyTestThing)
+    mock_thing._thing_server_interface = mocker.Mock()
+    descriptorinfo = prop.descriptor_info(mock_thing)
+    assert isinstance(descriptorinfo, PropertyInfo)
     for ann in prop_info.constraints:
         assert any(meta == ann for meta in m.model_fields["root"].metadata)
     for valid in prop_info.valid_values:
+        # Check the model can be created
         instance = m(root=valid)
+        # Check the value passes through the model
         validated = instance.model_dump()
         assert validated == valid or validated is valid  # `is` for NaN
+        # Check the descriptorinfo object also validates
+        # (this is what we get from thing.properties["name"])
+        validated = descriptorinfo.validate(valid)
+        assert validated == valid or validated is valid  # `is` for NaN
+        # Check that assignment works
+        prop.__set__(mock_thing, valid)
+        validated = prop.__get__(mock_thing)
+        assert validated == valid or validated is valid  # `is` for NaN
     for invalid in prop_info.invalid_values:
         with pytest.raises(ValidationError):
             _ = m(root=invalid)
+        with pytest.raises(ValidationError):
+            descriptorinfo.validate(invalid)
+        with pytest.raises(ValidationError):
+            prop.__set__(mock_thing, invalid)
 
 
 def convert_inf_nan(value):

tests/test_property.py

@@ -365,8 +365,8 @@ class BadIntModel(pydantic.BaseModel):
     # Check that a broken `_model` raises the right error
     # See above for where we manually set badprop._model to something that's
     # not a rootmodel.
-    example.badprop = 3
-    assert example.badprop == 3
+    with pytest.raises(TypeError):
+        example.badprop = 3  # Validation will fail here because of the bad model.
     with pytest.raises(TypeError):
         _ = example.properties["badprop"].model_instance
     with pytest.raises(TypeError):
@@ -391,12 +391,24 @@ class BadIntModel(pydantic.BaseModel):
     # Check validation for a model
     modelprop = example.properties["modelprop"]
     assert modelprop.validate(MyModel(a=3, b="four")) == MyModel(a=3, b="four")
+    # Check that a valid model passes through unchanged: this should indicate that
+    # we're not unnecessarily re-validating already-valid models.
     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 that an invalid model doesn't get re-validated. This is intended behaviour:
+    # it is another test that we're not unnecessarily re-validating a model that
+    # should already have been validated when it was created.
+    # Creating models with `model_construct` intentionally allows invalid models:
+    # if this is used downstream, the downstream code should accept responsibility!
+    bad_m = MyModel.model_construct(a="not an int", b=6)
+    assert modelprop.validate(bad_m) is bad_m
+    with pytest.raises(pydantic.ValidationError):
+        # Check that passing the same data in as a dict fails validation.
+        modelprop.validate(bad_m.model_dump())
 
     # Check again for an odd rootmodel
     rootmodelprop = example.properties["rootmodelprop"]
@@ -409,6 +421,11 @@ class BadIntModel(pydantic.BaseModel):
     for invalid in ["seven", {"root": None}, 14.5, pydantic.RootModel[int](root=0)]:
         with pytest.raises(pydantic.ValidationError):
             modelprop.validate(invalid)
+        # Tty constructing a model with an invalid root value, skipping validation
+        invalid_model = rootmodelprop.model.model_construct(invalid)
+        # The RootModel gets re-validated, in contrast to the BaseModel above.
+        with pytest.raises(pydantic.ValidationError):
+            assert modelprop.validate(invalid_model) == invalid
 
 
 def test_readonly_metadata():