diff --git a/src/labthings_fastapi/problem_details.py b/src/labthings_fastapi/problem_details.py index 0d7d1008..09c9d4ce 100644 --- a/src/labthings_fastapi/problem_details.py +++ b/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 diff --git a/src/labthings_fastapi/properties.py b/src/labthings_fastapi/properties.py index 4fcf19b2..2de6fc68 100644 --- a/src/labthings_fastapi/properties.py +++ b/src/labthings_fastapi/properties.py @@ -61,7 +61,8 @@ class attribute. Documentation is in strings immediately following the 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,7 +81,6 @@ class attribute. Documentation is in strings immediately following the ) from labthings_fastapi.exceptions import ( FeatureNotAvailableError, - InvalidReturnValueError, NotConnectedToServerError, PropertyRedefinitionError, ReadOnlyPropertyError, @@ -88,6 +88,7 @@ class attribute. Documentation is in strings immediately following the 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 ( @@ -555,10 +556,12 @@ def add_to_fastapi(self, app: FastAPI, thing: Owner) -> None: # 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 @@ def set_property(body: Any) -> None: 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 @@ def get_property() -> Response: 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 diff --git a/tests/test_problem_details.py b/tests/test_problem_details.py index 08c4cf55..44a1ac6d 100644 --- a/tests/test_problem_details.py +++ b/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" diff --git a/tests/test_properties.py b/tests/test_properties.py index daf71f84..f525bd9b 100644 --- a/tests/test_properties.py +++ b/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): diff --git a/tests/test_property.py b/tests/test_property.py index d5077d50..558a0cd0 100644 --- a/tests/test_property.py +++ b/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() diff --git a/tests/test_property_errors.py b/tests/test_property_errors.py new file mode 100644 index 00000000..f24b2a31 --- /dev/null +++ b/tests/test_property_errors.py @@ -0,0 +1,162 @@ +"""Test how errors in property getters/setters are handled. + +This test file is intended to check the error handling code dealing with properties. +Errors in property getter/setter code should result in helpful HTTP errors and +Python exceptions. This test module is intended to check that various error +conditions result in easily-interpreted exceptions, log entries, and HTTP responses. +""" + +import logging + +import pytest + +import labthings_fastapi as lt +from labthings_fastapi.exceptions import ClientPropertyError +from labthings_fastapi.testing import create_thing_without_server + + +class SpecificError(RuntimeError): + """A specific error class we can test for.""" + + +class ErrorThing(lt.Thing): + """A Thing that has properties with a variety of failure modes.""" + + @lt.property + def always_raises(self) -> int: + """An integer that errors on get and set.""" + raise SpecificError("'always_raises' failed, as expected.") + + @always_raises.setter + def _set_always_raises(self, value: int) -> None: + raise SpecificError("'always_raises' failed, as expected.") + + @always_raises.resetter + def _reset_always_raises(self) -> None: + raise SpecificError("'always_raises' failed, as expected.") + + @lt.property + def wrongly_typed(self) -> int: + """A value of the wrong type.""" + return "not an integer" + + @lt.property + def violates_constraint(self) -> int: + """An integer that's outside the allowed range.""" + return 42 + + violates_constraint.constraints = {"le": 10} + + +@pytest.fixture +def thing(): + """A fixture returning an instance of ErrorThing.""" + return create_thing_without_server(ErrorThing) + + +@pytest.fixture +def client(): + """A fixture returning a ThingClient for an ErrorThing.""" + server = lt.ThingServer.from_things({"thing": ErrorThing}) + with server.test_client() as tc: + return lt.ThingClient.from_url("/thing/", client=tc) + + +# The functions below check errors when called directly from Python + + +def test_get_always_raises_python(thing: ErrorThing): + """Check always_raises errors when retrieved directly.""" + with pytest.raises(SpecificError, match="failed, as expected"): + _ = thing.always_raises + + +def test_set_always_raises_python(thing: ErrorThing): + """Check always_raises errors when set directly.""" + with pytest.raises(SpecificError, match="failed, as expected"): + thing.always_raises = 42 + + +def test_reset_always_raises_python(thing: ErrorThing): + """Check always_raises errors when reset.""" + with pytest.raises(SpecificError, match="failed, as expected"): + thing.properties["always_raises"].reset() + + +def test_wrongly_typed_python(thing: ErrorThing): + """Check we can retrieve a wrongly typed value in Python.""" + # Currently, no validation is performed on property values + # when they are retrieved from Python + assert thing.wrongly_typed == "not an integer" + + +def test_violates_constraint_python(thing: ErrorThing): + """Check we can retrieve a value in Python that violates its constraint.""" + assert thing.violates_constraint == 42 + + +# The next section tests the same conditions as the previous one, but in the +# context of a server. +# Typing note: `client` is type hinted as an ErrorThing as it should have an +# equivalent signature. This is largely to enable static analysis in editors. +# It is actually a `lt.ThingClient` instance. + + +def test_get_always_raises_server(client: ErrorThing, caplog): + r"""Check always_raises errors nicely when retrieved over HTTP. + + Note that `caplog` here is capturing the *server* log, the client-side + code doesn't log, but does raise a `ClientPropertyError`\ . + """ + with pytest.raises(ClientPropertyError, match="failed, as expected"): + _ = client.always_raises + assert caplog.record_tuples == [ + ("labthings_fastapi.things.thing", 40, "'always_raises' failed, as expected."), + ] + + +def test_set_always_raises_server(client: ErrorThing, caplog): + """Check always_raises errors nicely when set over HTTP.""" + with pytest.raises(ClientPropertyError, match="failed, as expected"): + client.always_raises = 42 + assert caplog.record_tuples == [ + ("labthings_fastapi.things.thing", 40, "'always_raises' failed, as expected."), + ] + + +def test_reset_always_raises_server(client: lt.ThingClient, caplog): + """Check always_raises errors when reset over HTTP.""" + # Reset isn't yet exposed in ThingClient, so we do it manually. + response = client.client.post("/thing/always_raises/reset") + assert response.status_code == 500 + value = response.json() + assert value["detail"] == "'always_raises' failed, as expected." + assert value["title"] == "SpecificError" + + +def test_wrongly_typed_server(client: ErrorThing, caplog): + """Check the error when we return a wrongly typed value.""" + with pytest.raises( + ClientPropertyError, + match="Error validating thing.wrongly_typed", + ): + _ = client.wrongly_typed + assert len(caplog.records) == 1 + assert caplog.records[0].levelno == logging.ERROR + assert "Error validating thing.wrongly_typed" in caplog.records[0].getMessage() + + +def test_violates_constraint_server(client: ErrorThing, caplog): + """Check we can retrieve a value that violates its constraint. + + Constraints are not yet validated on the return values of property + getters. + """ + with pytest.raises( + ClientPropertyError, + match="Error validating thing.violates_constraint", + ): + _ = client.violates_constraint + assert len(caplog.records) == 1 + assert caplog.records[0].levelno == logging.ERROR + assert "validating thing.violates_constraint" in caplog.records[0].getMessage()