Property error handling

src/labthings_fastapi/problem_details.py

@@ -4,6 +4,12 @@
 "Problem Details" objects to represent errors in HTTP responses.
 """
 
+from functools import wraps
+from logging import Logger
+from typing import Any, Callable, ParamSpec, TypeVar
+
+from fastapi import Response
+from fastapi.responses import JSONResponse
 from pydantic import BaseModel, ConfigDict
 from typing_extensions import Self
 
@@ -40,6 +46,15 @@ def from_exception(cls, exc: BaseException) -> Self:
             status=getattr(exc, "status_code", 500),
         )
 
+    def json_response(self) -> Response:
+        """Return a JSONResponse representing this object.
+
+        :return: a `JSONResponse` object suitable for returning from a request handler.
+        """
+        # Typing note: this is typed as `Response` in case we move to using `pydantic`
+        # to serialise directly to JSON in the future.
+        return JSONResponse(self.model_dump(), status_code=self.status or 500)
+
 
 # This URL should describe all exceptions in this module.
 DOCS_URL = (
@@ -61,3 +76,45 @@ def docs_url(exc: type[BaseException]) -> str | None:
     if exc.__module__ == "builtins":
         return f"{PYTHON_DOCS_URL}#{exc.__name__}"
     return None
+
+
+Params = ParamSpec("Params")
+ReturnT = TypeVar("ReturnT", bound=Response)
+
+
+def exceptions_to_problem_details(
+    logger: Logger | None,
+) -> Callable[[Callable[Params, ReturnT]], Callable[Params, ReturnT]]:
+    """Decorate a function to handle errors with a `ProblemDetails` response.
+
+    :param logger: the logger to use, or `None` for no logging.
+    :return: a decorator that handles errors by returning a `ProblemDetails`
+        object.
+    """
+
+    def decorator(func: Callable[Params, ReturnT]) -> Callable[Params, ReturnT]:
+        """Wrap a function in error handling code.
+
+        This function is a decorator with no arguments, `logger` is available
+        as a local variable.
+
+        :param func: the function to be wrapped.
+        :return: the function, wrapped in error handling code.
+        """
+
+        @wraps(func)
+        def decorated(*args: Any, **kwargs: Any) -> Any:
+            # Typing note: the type hints and docstring should be supplied by @wraps.
+            try:
+                return func(*args, **kwargs)
+            except Exception as e:  # noqa: BLE001
+                # This decorator is an intentional catch-all error handler, so BLE001 is
+                # appropriately ignored here.
+                if logger:
+                    logger.error(e)
+                pd = ProblemDetails.from_exception(e)
+                return pd.json_response()
+
+        return decorated
+
+    return decorator

src/labthings_fastapi/properties.py

@@ -61,7 +61,8 @@
     overload,
 )
 
-from fastapi import Body, FastAPI, HTTPException, Response
+from fastapi import Body, FastAPI, Response
+from fastapi.responses import JSONResponse
 from pydantic import (
     BaseModel,
     ConfigDict,
@@ -80,14 +81,14 @@
 )
 from labthings_fastapi.exceptions import (
     FeatureNotAvailableError,
-    InvalidReturnValueError,
     NotConnectedToServerError,
     PropertyRedefinitionError,
     ReadOnlyPropertyError,
     UnserialisableTypeError,
     UnsupportedConstraintError,
 )
 from labthings_fastapi.message_broker import Message
+from labthings_fastapi.problem_details import exceptions_to_problem_details
 from labthings_fastapi.thing_class_settings import get_validate_properties_on_set
 from labthings_fastapi.thing_description import type_to_dataschema
 from labthings_fastapi.thing_description._model import (
@@ -498,7 +499,7 @@
         :return: the default value of this property.
         :raises FeatureNotAvailableError: as this must be overridden.
         """
         raise FeatureNotAvailableError(
             f"{obj.name if obj else self.__class__}.{self.name} can't return a "
             f"default, as it's not supported by {self.__class__}."
         )
@@ -516,7 +517,7 @@
         :param obj: the `~lt.Thing` instance we want to reset.
         :raises FeatureNotAvailableError: as only some subclasses implement resetting.
         """
         raise FeatureNotAvailableError(
             f"{obj.name}.{self.name} cannot be reset, as it's not supported by "
             f"{self.__class__}."
         )
@@ -555,10 +556,12 @@
             # The function is initially defined with a ``body`` argument of type
             # ``Any`` but this will be replaced with the correct annotation a
             # few lines below.
-            def set_property(body: Any) -> None:
+            @exceptions_to_problem_details(logger=thing.logger)
+            def set_property(body: Any) -> Response:
                 if isinstance(body, RootModel):
                     body = body.root
                 self.__set__(thing, body)
+                return JSONResponse(None, status_code=201)
 
             set_property.__annotations__["body"] = Annotated[self.model, Body()]
             app.put(
@@ -576,25 +579,19 @@
             summary=self.title,
             description=f"## {self.title}\n\n{self.description or ''}",
         )
+        @exceptions_to_problem_details(logger=thing.logger)
         def get_property() -> Response:
-            try:
-                instance = validate_from_user_code(
-                    model=self.model,
-                    value=self.__get__(thing),
-                    description=f"{thing.name}.{self.name}",
-                    code=(self.owning_class, self.name),
-                )
-                return serialise_from_user_code(
-                    model_instance=instance,
-                    description=f"{thing.name}.{self.name}",
-                    code=(self.owning_class, self.name),
-                )
-            except InvalidReturnValueError as e:
-                thing.logger.error(e)
-                raise HTTPException(
-                    status_code=500,
-                    detail=str(e),
-                ) from e
+            instance = validate_from_user_code(
+                model=self.model,
+                value=self.__get__(thing),
+                description=f"{thing.name}.{self.name}",
+                code=(self.owning_class, self.name),
+            )
+            return serialise_from_user_code(
+                model_instance=instance,
+                description=f"{thing.name}.{self.name}",
+                code=(self.owning_class, self.name),
+            )
 
         if self.is_resettable(thing):
 
@@ -612,8 +609,10 @@
                     rf"with the ``name`` argument set to ``{self.name}``\ ."
                 ),
             )
-            def reset() -> None:
+            @exceptions_to_problem_details(logger=thing.logger)
+            def reset() -> Response:
                 self.reset(thing)
+                return JSONResponse(None, status_code=200)
 
     def property_affordance(
         self, thing: Owner, path: str | None = None
@@ -967,7 +966,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.
@@ -990,7 +989,7 @@
         :raises ReadOnlyPropertyError: if the property cannot be set.
         """
         if self.fset is None:
             raise ReadOnlyPropertyError(f"Property {self.name} of {obj} has no setter.")
         if get_validate_properties_on_set(obj.__class__):
             property_info = self.descriptor_info(obj)
             value = property_info.validate(value)

tests/test_problem_details.py

@@ -1,6 +1,10 @@
 """Test we can generate problem details to describe exceptions nicely."""
 
+import httpx2
 import pytest
+from fastapi import FastAPI, Response
+from fastapi.responses import JSONResponse
+from fastapi.testclient import TestClient
 
 from labthings_fastapi import exceptions, problem_details
 
@@ -30,6 +34,21 @@ class CustomError(Exception):
 ]
 
 
+# There are some valid ProblemDetails instances that couldn't be made by
+# from_exception (for example, if detail or status is missing). We
+# extend the list of test cases to include these.
+PD_INSTANCES = [
+    *[
+        problem_details.ProblemDetails.from_exception(exc("Message"))
+        for exc, _url, _code in ERRORS
+    ],
+    problem_details.ProblemDetails(detail="Message"),
+    problem_details.ProblemDetails(detail="Message", status=501),
+    problem_details.ProblemDetails(),
+    problem_details.ProblemDetails(instance="Instance-specific message."),
+]
+
+
 @pytest.mark.parametrize(("err", "url", "_code"), ERRORS)
 def test_docs_url(err, url, _code):
     """Check URLs for built-in and LabThings errors, and that we get None for others."""
@@ -43,3 +62,81 @@ def test_pd_from_exception(err, url, code):
     assert pd.detail == "Message"
     assert pd.title == err.__name__
     assert pd.status == code
+
+
+def evaluate_response(response: Response) -> httpx2.Response:
+    """Use a TestClient to turn a Starlette Response into an httpx2 one.
+
+    This allows us to easily evaluate the body of a Response object.
+    This will create an ephemeral `TestClient` object. Doing so is not
+    massively efficient, but it doesn't slow down tests unduly and it
+    minimises custom code.
+
+    :param response: a `fastapi.Response` object.
+    :return a `httpx2.Response` object.
+    """
+    app = FastAPI()
+
+    @app.get("/")
+    def return_error() -> Response:
+        return response
+
+    with TestClient(app) as tc:
+        return tc.get("/")
+
+
+@pytest.mark.parametrize("pd", PD_INSTANCES)
+def test_response_from_exception(pd):
+    response = evaluate_response(pd.json_response())
+
+    assert response.status_code == pd.status or 500
+    value = response.json()
+    assert value == pd.model_dump()
+
+
+def test_exceptions_to_problem_details_noerror(mocker):
+    """Check the `exceptions_to_problem_details` decorator with no error."""
+
+    response = JSONResponse("success!", status_code=200)
+    logger = mocker.Mock()
+
+    @problem_details.exceptions_to_problem_details(logger=logger)
+    def successful():
+        return response
+
+    # The function should complete with no error and return the value
+    assert successful() is response
+
+    # Our mocked logger should record nothing
+    assert logger.error.called is False
+
+
+@pytest.mark.parametrize(("err", "url", "code"), ERRORS)
+def test_exceptions_to_problem_details_error(err, url, code, mocker):
+    """Check exceptions produce a response with the right message."""
+    logger = mocker.Mock()
+
+    @problem_details.exceptions_to_problem_details(logger=logger)
+    def fails():
+        raise err("Message")
+
+    if not issubclass(err, Exception):
+        # BaseException (and other non-Exception errors) isn't caught.
+        with pytest.raises(err):
+            fails()
+        return
+
+    # The function should complete, but the returned response describes
+    # the error.
+    response = evaluate_response(fails())
+    assert response.status_code == code
+    value = response.json()
+    assert value["type"] == url
+    assert value["detail"] == "Message"
+    assert value["title"] == err.__name__
+    assert value["status"] == code
+
+    # Our mocked logger should record an error
+    assert logger.error.call_count == 1
+    assert isinstance(logger.error.call_args[0][0], err)
+    assert str(logger.error.call_args[0][0]) == "Message"

tests/test_properties.py

@@ -281,7 +281,7 @@ def func_prop(self) -> MyModel:
     assert Dummy.func_prop.value_type is MyModel
 
 
-def test_property_get_and_set(server):
+def test_property_get_and_set_and_reset(server):
     """Use PUT and GET requests to check the property.
 
     PUT sets the value and GET retrieves it, so we use a PUT
@@ -294,10 +294,15 @@ def test_property_get_and_set(server):
         response = client.put("/thing/stringprop", json=test_str)
         # Check for a successful response code
         assert response.status_code == 201
+        assert response.content == b"null"
         # Check it was written successfully
         after_value = client.get("/thing/stringprop")
         assert after_value.status_code == 200
         assert after_value.json() == test_str
+        # Reset the property over HTTP
+        response = client.post("/thing/stringprop/reset")
+        assert response.status_code == 200
+        assert response.json() is None
 
 
 def test_boolprop(server):

tests/test_property.py

@@ -231,8 +231,9 @@ class Example:
         """
         prop._type = str | None
 
-        # Add a path attribute, so we can use Example as a mock Thing.
+        # Add path and logger, so we can use Example as a mock Thing.
         path = "/example/"
+        logger = None
 
     # Make a FastAPI app and retrieve the OpenAPI document
     app = fastapi.FastAPI()