Fix typing for actions.

src/labthings_fastapi/actions.py

@@ -18,7 +18,7 @@
 import datetime
 import logging
 from collections import deque
-from functools import partial, wraps
+from functools import partial
 import inspect
 from threading import Thread, Lock
 import uuid
@@ -27,20 +27,21 @@
     Annotated,
     Any,
     Callable,
+    Concatenate,
+    Generic,
     Optional,
-    Literal,
-    Union,
+    ParamSpec,
+    TypeVar,
     overload,
 )
 from weakref import WeakSet
 import weakref
 from fastapi import FastAPI, HTTPException, Request, Body, BackgroundTasks
 from pydantic import BaseModel, create_model
 
-from labthings_fastapi.logs import add_thing_log_destination
-
+from .base_descriptor import BaseDescriptor
+from .logs import add_thing_log_destination
 from .utilities import model_to_dict
-from .thing_description._model import LinkElement
 from .invocations import InvocationModel, InvocationStatus, LogRecordModel
 from .dependencies.invocation import NonWarningInvocationID
 from .exceptions import (
@@ -55,13 +56,11 @@
     EmptyInput,
     StrictEmptyInput,
     fastapi_dependency_params,
-    get_docstring,
-    get_summary,
     input_model_from_signature,
     return_type,
 )
 from .thing_description import type_to_dataschema
-from .thing_description._model import ActionAffordance, ActionOp, Form
+from .thing_description._model import ActionAffordance, ActionOp, Form, LinkElement
 from .utilities import labthings_data
 
 
@@ -160,8 +159,8 @@
         """
         try:
             blobdata_to_url_ctx.get()
         except LookupError as e:
             raise NoBlobManagerError(
                 "An invocation output has been requested from a api route that "
                 "doesn't have a BlobIOContextDep dependency. This dependency is needed "
                 " for blobs to identify their url."
@@ -412,8 +411,8 @@
         :param id: the unique ID of the action to retrieve.
         :return: the `.Invocation` object.
         """
         with self._invocations_lock:
             return self._invocations[id]
 
     def list_invocations(
         self,
@@ -541,8 +540,8 @@
             with self._invocations_lock:
                 try:
                     invocation: Any = self._invocations[id]
                 except KeyError as e:
                     raise HTTPException(
                         status_code=404,
                         detail="No action invocation found with ID {id}",
                     ) from e
@@ -555,7 +554,7 @@
                     invocation.output.response
                 ):
                     # TODO: honour "accept" header
                     return invocation.output.response()
                 return invocation.output
 
         @app.delete(
@@ -580,8 +579,8 @@
             with self._invocations_lock:
                 try:
                     invocation: Any = self._invocations[id]
                 except KeyError as e:
                     raise HTTPException(
                         status_code=404,
                         detail="No action invocation found with ID {id}",
                     ) from e
@@ -622,7 +621,15 @@
 """
 
 
-class ActionDescriptor:
+ActionParams = ParamSpec("ActionParams")
+ActionReturn = TypeVar("ActionReturn")
+OwnerT = TypeVar("OwnerT", bound="Thing")
+
+
+class ActionDescriptor(
+    BaseDescriptor[Callable[ActionParams, ActionReturn]],
+    Generic[ActionParams, ActionReturn, OwnerT],
+):
     """Wrap actions to enable them to be run over HTTP.
 
     This class is responsible for generating the action description for
@@ -640,7 +647,7 @@
 
     def __init__(
         self,
-        func: Callable,
+        func: Callable[Concatenate[OwnerT, ActionParams], ActionReturn],
         response_timeout: float = 1,
         retention_time: float = 300,
     ) -> None:
@@ -662,7 +669,12 @@
         :param retention_time: how long, in seconds, the action should be kept
             for after it has completed.
         """
+        super().__init__()
         self.func = func
+        if func.__doc__ is not None:
+            # Use the docstring from the function, if there is one.
+            self.__doc__ = func.__doc__
+        name = func.__name__  # this is checked in __set_name__
         self.response_timeout = response_timeout
         self.retention_time = retention_time
         self.dependency_params = fastapi_dependency_params(func)
@@ -673,63 +685,47 @@
         )
         self.output_model = return_type(func)
         self.invocation_model = create_model(
-            f"{self.name}_invocation",
+            f"{name}_invocation",
             __base__=InvocationModel,
             input=(self.input_model, ...),
             output=(Optional[self.output_model], None),
         )
-        self.invocation_model.__name__ = f"{self.name}_invocation"
+        self.invocation_model.__name__ = f"{name}_invocation"
 
-    @overload
-    def __get__(self, obj: Literal[None], type: type[Thing]) -> ActionDescriptor:  # noqa: D105
-        ...
+    def __set_name__(self, owner: type[Thing], name: str) -> None:
+        """Ensure the action name matches the function name.
 
-    @overload
-    def __get__(self, obj: Thing, type: type[Thing] | None = None) -> Callable:  # noqa: D105
-        ...
+        It's assumed in a few places that the function name and the
+        descriptor's name are the same. This should always be the case
+        if it's used as a decorator.
+
+        :param owner: The class owning the descriptor.
+        :param name: The name of the descriptor in the class.
+        :raises ValueError: if the action name does not match the function name.
+        """
+        super().__set_name__(owner, name)
+        if self.name != self.func.__name__:
+            raise ValueError(
+                f"Action name '{self.name}' does not match function name "
+                f"'{self.func.__name__}'",
+            )
 
-    def __get__(
-        self, obj: Optional[Thing], type: Optional[type[Thing]] = None
-    ) -> Union[ActionDescriptor, Callable]:
+    def instance_get(self, obj: Thing) -> Callable[ActionParams, ActionReturn]:
         """Return the function, bound to an object as for a normal method.
 
         This currently doesn't validate the arguments, though it may do so
         in future. In its present form, this is equivalent to a regular
         Python method, i.e. all we do is supply the first argument, `self`.
 
-        If `obj` is None, the descriptor is returned, so we can get
-        the descriptor conveniently as an attribute of the class.
-
         :param obj: the `.Thing` to which we are attached. This will be
             the first argument supplied to the function wrapped by this
             descriptor.
-        :param type: the class of the `.Thing` to which we are attached.
-            If the descriptor is accessed via the class it is returned
-            directly.
-        :return: the action function, bound to ``obj`` (when accessed
-            via an instance), or the descriptor (accessed via the class).
+        :return: the action function, bound to ``obj``.
         """
-        if obj is None:
-            return self
-        # TODO: do we attempt dependency injection here? I think not.
-        # If we want dependency injection, we should be calling the action
-        # via some sort of client object.
-        return partial(self.func, obj)
-
-    @property
-    def name(self) -> str:
-        """The name of the wrapped function."""
-        return self.func.__name__
-
-    @property
-    def title(self) -> str:
-        """A human-readable title."""
-        return get_summary(self.func) or self.name
-
-    @property
-    def description(self) -> str | None:
-        """A description of the action."""
-        return get_docstring(self.func, remove_summary=True)
+        # `obj` should be of type `OwnerT`, but `BaseDescriptor` currently
+        # isn't generic in the type of the owning Thing, so we can't express
+        # that here.
+        return partial(self.func, obj)  # type: ignore[arg-type]
 
     def _observers_set(self, obj: Thing) -> WeakSet:
         """Return a set used to notify changes.
@@ -858,14 +854,14 @@
         try:
             responses[200]["model"] = self.output_model
             pass
         except AttributeError:
             print(f"Failed to generate response model for action {self.name}")
         # Add an additional media type if we may return a file
         if hasattr(self.output_model, "media_type"):
             responses[200]["content"][self.output_model.media_type] = {}
         # Now we can add the endpoint to the app.
         if thing.path is None:
             raise NotConnectedToServerError(
                 "Can't add the endpoint without thing.path!"
             )
         app.post(
@@ -913,7 +909,7 @@
         """
         path = path or thing.path
         if path is None:
             raise NotConnectedToServerError("Can't generate forms without a path!")
         forms = [
             Form[ActionOp](href=path + self.name, op=[ActionOp.invokeaction]),
         ]
@@ -926,50 +922,33 @@
         )
 
 
-def mark_action(func: Callable, **kwargs: Any) -> ActionDescriptor:
-    r"""Mark a method of a Thing as an Action.
-
-    We replace the function with a descriptor that's a
-    subclass of `.ActionDescriptor`
-
-    :param func: The function to be decorated.
-    :param \**kwargs: Additional keyword arguments are passed to the constructor
-        of `.ActionDescriptor`.
-
-    :return: An `.ActionDescriptor` wrapping the method.
-    """
-
-    class ActionDescriptorSubclass(ActionDescriptor):
-        pass
-
-    return ActionDescriptorSubclass(func, **kwargs)
-
-
 @overload
-def action(func: Callable, **kwargs: Any) -> ActionDescriptor: ...
+def action(
+    func: Callable[Concatenate[OwnerT, ActionParams], ActionReturn], **kwargs: Any
+) -> ActionDescriptor[ActionParams, ActionReturn, OwnerT]: ...
 
 
 @overload
 def action(
     **kwargs: Any,
 ) -> Callable[
     [
-        Callable,
+        Callable[Concatenate[OwnerT, ActionParams], ActionReturn],
     ],
-    ActionDescriptor,
+    ActionDescriptor[ActionParams, ActionReturn, OwnerT],
 ]: ...
 
 
-@wraps(mark_action)
 def action(
-    func: Optional[Callable] = None, **kwargs: Any
+    func: Callable[Concatenate[OwnerT, ActionParams], ActionReturn] | None = None,
+    **kwargs: Any,
 ) -> (
-    ActionDescriptor
+    ActionDescriptor[ActionParams, ActionReturn, OwnerT]
     | Callable[
         [
-            Callable,
+            Callable[Concatenate[OwnerT, ActionParams], ActionReturn],
         ],
-        ActionDescriptor,
+        ActionDescriptor[ActionParams, ActionReturn, OwnerT],
     ]
 ):
     r"""Mark a method of a `.Thing` as a LabThings Action.
@@ -996,6 +975,6 @@
     # return a partial object, which then calls the
     # wrapped function once.
     if func is not None:
-        return mark_action(func, **kwargs)
+        return ActionDescriptor(func, **kwargs)
     else:
-        return partial(mark_action, **kwargs)
+        return partial(ActionDescriptor, **kwargs)

tests/test_actions.py

@@ -206,3 +206,83 @@ def decorated(
     # Check we can make the thing and it has a valid TD
     example = create_thing_without_server(Example)
     example.validate_thing_description()
+
+
+def test_action_docs():
+    """Check that action documentation is included in the Thing Description.
+
+    This test was added to check that the generated documentation is correct,
+    after some refactoring of `lt.action`.
+
+    `name`, `title` and `description` attributes are now handled by `BaseDescriptor`
+    and are tested more extensively there - but it seemed worthwhile to have some
+    tests of them in the context of actions.
+    """
+
+    class DocThing(lt.Thing):
+        @lt.action
+        def documented_action(self) -> None:
+            """This is the action docstring."""
+            pass
+
+        @lt.action
+        def convert_type(self, a: int) -> float:
+            """Convert an integer to a float."""
+            return float(a)
+
+        @lt.action
+        def no_doc_action(self) -> None:
+            pass
+
+        @lt.action
+        def long_docstring(self) -> None:
+            """Do something with a very long docstring.
+
+            It has multiple paragraphs.
+
+            Here is the second paragraph.
+
+            And here is the third.
+            """
+            pass
+
+    # Create a Thing, and generate the Thing Description. This uses `BaseDescriptor`
+    # functionality to extract the name, title, and description.
+    thing = create_thing_without_server(DocThing)
+    td = thing.thing_description()
+    actions = td.actions
+    # The various `assert <whatever> is not None` statements are mostly for type
+    # checking/autocompletion while writing the tests.
+    assert actions is not None
+
+    # The function docstring should propagate through as the description.
+    assert actions["documented_action"].description == "This is the action docstring."
+
+    # It's important that we check more than one action, to ensure there is no
+    # "leakage" between instances. Previous implementations always subclassed
+    # `ActionDescriptor` to avoid leakage when methods were manipulated at runtime.
+    # This is no longer done, so we can instantiate `ActionDescriptor` directly - but
+    # it's good to make sure we can have multiple actions with different docstrings.
+    assert actions["convert_type"].description == "Convert an integer to a float."
+
+    # convert_type also allows us to check that the action inputs and outputs are
+    # correctly represented in the thing description.
+    input = actions["convert_type"].input
+    assert input is not None
+    input_properties = input.properties
+    assert input_properties is not None
+    assert input_properties["a"].type.value == "integer"
+    output = actions["convert_type"].output
+    assert output is not None
+    assert output.type.value == "number"
+
+    # An action with no docstring should have no description, and a default title.
+    assert actions["no_doc_action"].description is None
+    assert actions["no_doc_action"].title == "no_doc_action"
+
+    # An action with a long docstring should have the docstring body as description,
+    # and the first line as title.
+    assert actions["long_docstring"].title == "Do something with a very long docstring."
+    assert actions["long_docstring"].description.startswith(
+        "It has multiple paragraphs."
+    )

typing_tests/README.md

@@ -1,11 +1,7 @@
 # Typing tests: check `labthings_fastapi` plays nicely with `mypy`.
 
-The codebase is type-checked with `mypy src/` and tested with `pytest`, however neither of these explicitly check that `mypy` can infer the correct types for `Thing` attributes like properties and actions. The Python files in this folder are intended to be checked using:
+The codebase is type-checked with `mypy` and tested with `pytest`, however neither of these explicitly check that `mypy` can infer the correct types for `Thing` attributes like properties and actions. The Python files in this folder are checked when we run `mypy`, and are intended to test that `Thing` code may be statically type-checked.
 
-```terminal
-mypy --warn-unused-ignores typing_tests
-```
-
-The files include valid code that's accompanied by `assert_type` statements (which check the inferred types are what we expect them to be) as well as invalid code where the expected `mypy` errors are ignored. This tests for expected errors - if an expected error is not thrown, it will cause an `unused-ignore` error.
+The files include valid code that's accompanied by `assert_type` statements (which check the inferred types are what we expect them to be) as well as invalid code where the expected `mypy` errors are ignored. This tests for expected errors - if an expected error is not thrown, it will cause an `unused-ignore` error as per the configuration in `pyproject.toml`.
 
 There are more elaborate type testing solutions available, but all of them add dependencies and require us to learn a new tool. This folder of "tests" feels like a reasonable way to test that the package plays well with static type checking, without too much added complication.