Add unit tests and docstrings. Also add pathcheck when addign a thing

Author: julianstirling

pyproject.toml

@@ -51,6 +51,14 @@ artifacts = ["src/*.json"]
 [tool.hatch.build.targets.wheel]
 artifacts = ["src/*.json"]
 
+[tool.pytest.ini_options]
+addopts = [
+    "--cov=labthings_fastapi",
+    "--cov-report=term",
+    "--cov-report=xml:coverage.xml",
+    "--cov-report=html:htmlcov",
+]
+
 [tool.ruff]
 target-version = "py310"
 

src/labthings_fastapi/descriptors/action.py

@@ -6,8 +6,11 @@
 from functools import partial
 import inspect
 from typing import TYPE_CHECKING, Annotated, Any, Callable, Optional, Literal, overload
+from weakref import WeakSet
+
 from fastapi import Body, FastAPI, Request, BackgroundTasks
 from pydantic import create_model
+
 from ..actions import InvocationModel
 from ..dependencies.invocation import CancelHook, InvocationID
 from ..dependencies.action_manager import ActionManagerContextDep
@@ -23,9 +26,8 @@
 from ..outputs.blob import BlobIOContextDep
 from ..thing_description import type_to_dataschema
 from ..thing_description.model import ActionAffordance, ActionOp, Form, Union
-
-from weakref import WeakSet
 from ..utilities import labthings_data, get_blocking_portal
+from ..exceptions import NotConnectedToServerError
 
 if TYPE_CHECKING:
     from ..thing import Thing
@@ -129,12 +131,23 @@ def _observers_set(self, obj):
     def emit_changed_event(self, obj, status):
         """Notify subscribers that the action status has changed
 
-        NB this function **must** be run from a thread, not the event loop.
+        This function is run from within the `Invocation` thread that
+        is created when an action is called. It must be run from this thread
+        as it is communicating with the event loop via an `asyncio` blocking
+        portal.
+
+        :raises NotConnectedToServerError: if the Thing calling the action is not
+        connected to a server with a running event loop.
         """
         try:
             runner = get_blocking_portal(obj)
             if not runner:
-                raise RuntimeError("Can't emit without a blocking portal")
+                thing_name = obj.__class__.__name__
+                msg = (
+                    f"Cannot emit action changed event. Is {thing_name} connected to "
+                    "a running server?"
+                )
+                raise NotConnectedToServerError(msg)
             runner.start_task_soon(
                 self.emit_changed_event_async,
                 obj,

src/labthings_fastapi/descriptors/property.py

@@ -4,14 +4,17 @@
 
 from __future__ import annotations
 from typing import TYPE_CHECKING, Annotated, Any, Callable, Optional
+from weakref import WeakSet
+
 from typing_extensions import Self
-from labthings_fastapi.utilities.introspection import get_summary, get_docstring
 from pydantic import BaseModel, RootModel
 from fastapi import Body, FastAPI
-from weakref import WeakSet
+
 from ..utilities import labthings_data, wrap_plain_types_in_rootmodel
+from ..utilities.introspection import get_summary, get_docstring
 from ..thing_description.model import PropertyAffordance, Form, DataSchema, PropertyOp
 from ..thing_description import type_to_dataschema
+from ..exceptions import NotConnectedToServerError
 
 
 if TYPE_CHECKING:
@@ -118,11 +121,18 @@ def _observers_set(self, obj):
     def emit_changed_event(self, obj: Thing, value: Any) -> None:
         """Notify subscribers that the property has changed
 
-        NB this function **must** be run from a thread, not the event loop.
+        This function is run when properties are upadated. It must be run from
+        within a thread. This could be the `Invocation` thread of a running action, or
+        the property should be updated over via a client/http. It must be run from a
+        thread as it is communicating with the event loop via an `asyncio` blocking
+        portal.
+
+        :raises NotConnectedToServerError: if the Thing that is calling the property
+        update is not connected to a server with a running event loop.
         """
         runner = obj._labthings_blocking_portal
         if not runner:
-            raise RuntimeError("Can't emit without a blocking portal")
+            raise NotConnectedToServerError("Can't emit without a blocking portal")
         runner.start_task_soon(
             self.emit_changed_event_async,
             obj,

src/labthings_fastapi/exceptions.py

@@ -0,0 +1,11 @@
+"""A submodule for custom LabThings-FastAPI Exceptions"""
+
+
+class NotConnectedToServerError(RuntimeError):
+    """The Thing is not connected to a server
+
+    This exception is called if a ThingAction is called or
+    is a ThingProperty is updated on a Thing that is not
+    connected to a ThingServer. A server connection is needed
+    to manage asynchronous behaviour.
+    """

src/labthings_fastapi/server/__init__.py

@@ -1,6 +1,8 @@
 from __future__ import annotations
 from typing import Optional, Sequence, TypeVar
 import os.path
+import re
+
 from fastapi import FastAPI, Request
 from fastapi.middleware.cors import CORSMiddleware
 from anyio.from_thread import BlockingPortal
@@ -17,6 +19,11 @@
 from ..dependencies.thing_server import _thing_servers
 from ..outputs.blob import BlobDataManager
 
+# A path should be made up of names separated by / as a path separator.
+# Each name should be made of alphanumeric characters, hyphen, or underscore.
+# This regex enforces a trailing /
+PATH_REGEX = re.compile(r"^/([a-zA-Z0-9\-_]+\/)+$")
+
 
 class ThingServer:
     def __init__(self, settings_folder: Optional[str] = None):
@@ -71,13 +78,26 @@ def thing_by_class(self, cls: type[ThingInstance]) -> ThingInstance:
         )
 
     def add_thing(self, thing: Thing, path: str):
-        """Add a thing to the server"""
+        """Add a thing to the server
+
+        :param thing: The thing to add to the server.
+        :param path: the relative path to access the thing on the server. Must only
+        contain alphanumeric characters, hyphens, or underscores.
+        """
+        # Ensure leading and trailing /
         if not path.endswith("/"):
             path += "/"
+        if not path.startswith("/"):
+            path = "/" + path
+        if PATH_REGEX.match(path) is None:
+            msg = (
+                f"{path} contains unsafe characters. Use only alphanumeric "
+                "characters, hyphens and underscores"
+            )
+            raise ValueError(msg)
         if path in self._things:
             raise KeyError(f"{path} has already been added to this thing server.")
         self._things[path] = thing
-        # TODO: check for illegal things in `path` - potential security issue.
         settings_folder = os.path.join(self.settings_folder, path.lstrip("/"))
         os.makedirs(settings_folder, exist_ok=True)
         thing.attach_to_server(

tests/test_properties.py

@@ -1,11 +1,14 @@
+from threading import Thread
+
+from pytest import raises
+from pydantic import BaseModel
+from fastapi.testclient import TestClient
+
 from labthings_fastapi.descriptors import ThingProperty
 from labthings_fastapi.decorators import thing_property, thing_action
 from labthings_fastapi.thing import Thing
-from fastapi.testclient import TestClient
 from labthings_fastapi.server import ThingServer
-from threading import Thread
-from pytest import raises
-from pydantic import BaseModel
+from labthings_fastapi.exceptions import NotConnectedToServerError
 
 
 class TestThing(Thing):
@@ -44,6 +47,12 @@ def toggle_boolprop_from_thread(self):
 
 
 def test_instantiation_with_type():
+    """
+    Check the internal model (data type) of the ThingSetting descriptor is a BaseModel
+
+    To send the data over HTTP LabThings-FastAPI uses Pydantic models to describe data
+    types.
+    """
     prop = ThingProperty(bool, False)
     assert issubclass(prop.model, BaseModel)
 
@@ -117,8 +126,11 @@ def test_setting_from_thread():
 
 
 def test_setting_without_event_loop():
+    """Test that an exception is raised if updating a ThingProperty
+    without connecting the Thing to a running server with an event loop.
+    """
     # This test may need to change, if we change the intended behaviour
     # Currently it should never be necessary to change properties from the
     # main thread, so we raise an error if you try to do so
-    with raises(RuntimeError):
+    with raises(NotConnectedToServerError):
         thing.boolprop = False  # Can't call it until the event loop's running

tests/test_settings.py

@@ -0,0 +1,155 @@
+from threading import Thread
+import tempfile
+import json
+import pytest
+import os
+import logging
+
+from fastapi.testclient import TestClient
+
+from labthings_fastapi.descriptors import ThingSetting
+from labthings_fastapi.decorators import thing_setting, thing_action
+from labthings_fastapi.thing import Thing
+from labthings_fastapi.server import ThingServer
+
+
+class TestThing(Thing):
+    boolsetting = ThingSetting(bool, False, description="A boolean setting")
+    stringsetting = ThingSetting(str, "foo", description="A string setting")
+
+    _float = 1.0
+
+    @thing_setting
+    def floatsetting(self) -> float:
+        return self._float
+
+    @floatsetting.setter
+    def floatsetting(self, value: float):
+        self._float = value
+
+    @thing_action
+    def toggle_boolsetting(self):
+        self.boolsetting = not self.boolsetting
+
+    @thing_action
+    def toggle_boolsetting_from_thread(self):
+        t = Thread(target=self.toggle_boolsetting)
+        t.start()
+
+
+def _get_setting_file(server, thingpath):
+    path = os.path.join(server.settings_folder, thingpath.lstrip("/"), "settings.json")
+    return os.path.normpath(path)
+
+
+def _settings_dict(boolsetting=False, floatsetting=1.0, stringsetting="foo"):
+    """Return the expected settings dictionary
+
+    Parameters can be updated from default values
+    """
+    return {
+        "boolsetting": boolsetting,
+        "floatsetting": floatsetting,
+        "stringsetting": stringsetting,
+    }
+
+
+@pytest.fixture
+def thing():
+    return TestThing()
+
+
+@pytest.fixture
+def server():
+    with tempfile.TemporaryDirectory() as tempdir:
+        # Yield server rather than return so that the temp directory isn't cleaned up
+        # until after the test is run
+        yield ThingServer(settings_folder=tempdir)
+
+
+def test_setting_available(thing):
+    """Check default settings are available before connecting to server"""
+    assert not thing.boolsetting
+    assert thing.stringsetting == "foo"
+    assert thing.floatsetting == 1.0
+
+
+def test_settings_save(thing, server):
+    """Check updated settings are saved to disk"""
+    setting_file = _get_setting_file(server, "/thing")
+    server.add_thing(thing, "/thing")
+    # No setting file created when first added
+    assert not os.path.isfile(setting_file)
+    with TestClient(server.app) as client:
+        r = client.put("/thing/floatsetting", json=2.0)
+        assert r.status_code == 201
+        r = client.get("/thing/floatsetting")
+        assert r.json() == 2.0
+        assert os.path.isfile(setting_file)
+        with open(setting_file, "r", encoding="utf-8") as file_obj:
+            # Check settings on file match expected dictionary
+            assert json.load(file_obj) == _settings_dict(floatsetting=2.0)
+
+
+def test_settings_load(thing, server):
+    """Check settings can be loaded from disk when added to server"""
+    setting_file = _get_setting_file(server, "/thing")
+    setting_json = json.dumps(_settings_dict(floatsetting=3.0, stringsetting="bar"))
+    # Create setting file
+    os.makedirs(os.path.dirname(setting_file))
+    with open(setting_file, "w", encoding="utf-8") as file_obj:
+        file_obj.write(setting_json)
+    # Add thing to server and check new settings are loaded
+    server.add_thing(thing, "/thing")
+    assert not thing.boolsetting
+    assert thing.stringsetting == "bar"
+    assert thing.floatsetting == 3.0
+
+
+def test_load_extra_settings(thing, server, caplog):
+    """Load from setting file. Extra setting in file should create a warning."""
+    setting_file = _get_setting_file(server, "/thing")
+    setting_dict = _settings_dict(floatsetting=3.0, stringsetting="bar")
+    setting_dict["extra_setting"] = 33.33
+    setting_json = json.dumps(setting_dict)
+    # Create setting file
+    os.makedirs(os.path.dirname(setting_file))
+    with open(setting_file, "w", encoding="utf-8") as file_obj:
+        file_obj.write(setting_json)
+
+    with caplog.at_level(logging.WARNING):
+        # Add thing to server
+        server.add_thing(thing, "/thing")
+        assert len(caplog.records) == 1
+        assert caplog.records[0].levelname == "WARNING"
+        assert caplog.records[0].name == "labthings_fastapi.thing"
+
+    # Check other settings are loaded as expected
+    assert not thing.boolsetting
+    assert thing.stringsetting == "bar"
+    assert thing.floatsetting == 3.0
+
+
+def test_try_loading_corrupt_settings(thing, server, caplog):
+    """Load from setting file. Extra setting in file should create a warning."""
+    setting_file = _get_setting_file(server, "/thing")
+    setting_dict = _settings_dict(floatsetting=3.0, stringsetting="bar")
+    setting_json = json.dumps(setting_dict)
+    # Cut the start off the json to so it can't be decoded.
+    setting_json = setting_json[3:]
+    # Create setting file
+    os.makedirs(os.path.dirname(setting_file))
+    with open(setting_file, "w", encoding="utf-8") as file_obj:
+        file_obj.write(setting_json)
+
+    with caplog.at_level(logging.WARNING):
+        # Add thing to server
+        server.add_thing(thing, "/thing")
+        assert len(caplog.records) == 1
+        assert caplog.records[0].levelname == "WARNING"
+        assert caplog.records[0].name == "labthings_fastapi.thing"
+
+    # Check default settings are loaded
+    assert not thing.boolsetting
+    assert thing.stringsetting == "foo"
+    assert thing.floatsetting == 1.0

tests/test_thing.py

@@ -1,7 +1,25 @@
+import pytest
 from labthings_fastapi.example_things import MyThing
+from labthings_fastapi.server import ThingServer
 
 
 def test_td_validates():
     """This will raise an exception if it doesn't validate OK"""
     thing = MyThing()
     assert thing.validate_thing_description() is None
+
+
+def test_add_thing():
+    """Check that thing can be added to the server"""
+    thing = MyThing()
+    server = ThingServer()
+    server.add_thing(thing, "/thing")
+
+
+def test_add_naughty_thing():
+    """Check that a thing trying to access server resources
+    using .. is not allowed"""
+    thing = MyThing()
+    server = ThingServer()
+    with pytest.raises(ValueError):
+        server.add_thing(thing, "/../../../../bin")