Merge pull request #130 from labthings/retire-file-manager

Remove the file_manager module

Author: rwb27

.coveragerc

@@ -3,3 +3,8 @@ concurrency = multiprocessing, thread
 parallel = true
 sigterm = true
 omit = tests/**/*.py, docs/**/*.py
+
+[report]
+exclude_lines = 
+    pragma: no cover
+    if TYPE_CHECKING:

src/labthings_fastapi/actions/__init__.py

@@ -8,13 +8,11 @@
 from typing import TYPE_CHECKING
 import weakref
 from fastapi import FastAPI, HTTPException, Request
-from fastapi.responses import FileResponse
 from pydantic import BaseModel
 
 from ..utilities import model_to_dict
 from ..utilities.introspection import EmptyInput
 from ..thing_description.model import LinkElement
-from ..file_manager import FileManager
 from .invocation_model import InvocationModel, InvocationStatus
 from ..dependencies.invocation import (
     CancelHook,
@@ -69,10 +67,6 @@ def __init__(
         self.retention_time = action.retention_time
         self.expiry_time: Optional[datetime.datetime] = None
 
-        # This is added post-hoc by the FastAPI endpoint, in
-        # `ActionDescriptor.add_to_fastapi`
-        self._file_manager: Optional[FileManager] = None
-
         # Private state properties
         self._status_lock = Lock()  # This Lock protects properties below
         self._status: InvocationStatus = InvocationStatus.PENDING  # Task status
@@ -148,8 +142,6 @@ def response(self, request: Optional[Request] = None):
             LinkElement(rel="self", href=href),
             LinkElement(rel="output", href=href + "/output"),
         ]
-        if self._file_manager:
-            links += self._file_manager.links(href)
         return self.action.invocation_model(
             status=self.status,
             id=self.id,
@@ -413,50 +405,3 @@ def delete_invocation(id: uuid.UUID) -> None:
                         ),
                     )
                 invocation.cancel()
-
-        @app.get(
-            ACTION_INVOCATIONS_PATH + "/{id}/files",
-            responses={
-                404: {"description": "Invocation ID not found"},
-                503: {"description": "No files are available for this invocation"},
-            },
-        )
-        def action_invocation_files(id: uuid.UUID) -> list[str]:
-            with self._invocations_lock:
-                try:
-                    invocation: Any = self._invocations[id]
-                except KeyError:
-                    raise HTTPException(
-                        status_code=404,
-                        detail="No action invocation found with ID {id}",
-                    )
-                if not invocation._file_manager:
-                    raise HTTPException(
-                        status_code=503,
-                        detail="No files are available for this invocation",
-                    )
-                return invocation._file_manager.filenames
-
-        @app.get(
-            ACTION_INVOCATIONS_PATH + "/{id}/files/{filename}",
-            response_class=FileResponse,
-            responses={
-                404: {"description": "Invocation ID not found, or file not found"},
-                503: {"description": "No files are available for this invocation"},
-            },
-        )
-        def action_invocation_file(id: uuid.UUID, filename: str):
-            with self._invocations_lock:
-                try:
-                    invocation: Any = self._invocations[id]
-                except KeyError:
-                    raise HTTPException(
-                        status_code=404,
-                        detail="No action invocation found with ID {id}",
-                    )
-                if not invocation._file_manager:
-                    raise HTTPException(
-                        status_code=503,
-                        detail="No files are available for this invocation",
-                    )
-                return FileResponse(invocation._file_manager.path(filename))

src/labthings_fastapi/descriptors/action.py

@@ -186,22 +186,16 @@ def start_action(
             background_tasks: BackgroundTasks,
             **dependencies,
         ):
-            try:
-                action = action_manager.invoke_action(
-                    action=self,
-                    thing=thing,
-                    input=body,
-                    dependencies=dependencies,
-                    id=id,
-                    cancel_hook=cancel_hook,
-                )
-                background_tasks.add_task(action_manager.expire_invocations)
-                return action.response(request=request)
-            finally:
-                try:
-                    action._file_manager = request.state.file_manager
-                except AttributeError:
-                    pass  # This probably means there was no FileManager created.
+            action = action_manager.invoke_action(
+                action=self,
+                thing=thing,
+                input=body,
+                dependencies=dependencies,
+                id=id,
+                cancel_hook=cancel_hook,
+            )
+            background_tasks.add_task(action_manager.expire_invocations)
+            return action.response(request=request)
 
         if issubclass(self.input_model, EmptyInput):
             annotation = Body(default_factory=StrictEmptyInput)

src/labthings_fastapi/file_manager.py

@@ -4,6 +4,7 @@
 from temp_client import poll_task
 import time
 import labthings_fastapi as lt
+from labthings_fastapi.actions import ACTION_INVOCATIONS_PATH
 
 
 class TestThing(lt.Thing):
@@ -22,16 +23,20 @@ def increment_counter(self):
 server.add_thing(thing, "/thing")
 
 
-def action_partial(client: TestClient, url: str):
-    def run(payload=None):
-        r = client.post(url, json=payload)
-        assert r.status_code in (200, 201)
-        return poll_task(client, r.json())
+def test_action_expires():
+    """Check the action is removed from the server
 
-    return run
+    We've set the retention period to be very short, so the action
+    should not be retrievable after some time has elapsed.
 
+    This test checks that actions do indeed get removed.
 
-def test_expiry():
+    Note that the code that expires actions runs whenever a new
+    action is started. That's why we need to invoke the action twice:
+    the second invocation runs the code that deletes the first one.
+    This behaviour might change in the future, making the second run
+    unnecessary.
+    """
     with TestClient(server.app) as client:
         before_value = client.get("/thing/counter").json()
         r = client.post("/thing/increment_counter")
@@ -42,5 +47,24 @@ def test_expiry():
         after_value = client.get("/thing/counter").json()
         assert after_value == before_value + 2
         invocation["status"] = "running"  # Force an extra poll
+        # When the second action runs, the first one should expire
+        # so polling it again should give a 404.
         with pytest.raises(httpx.HTTPStatusError):
             poll_task(client, invocation)
+
+
+def test_actions_list():
+    """Check that the /action_invocations/ path works.
+
+    The /action_invocations/ path should return a list of invocation
+    objects (a representation of each action that's been run recently).
+
+    It's implemented in `ActionManager.list_all_invocations`.
+    """
+    with TestClient(server.app) as client:
+        r = client.post("/thing/increment_counter")
+        invocation = poll_task(client, r.json())
+        r2 = client.get(ACTION_INVOCATIONS_PATH)
+        r2.raise_for_status()
+        invocations = r2.json()
+        assert invocations == [invocation]

tests/test_action_manager.py

@@ -6,7 +6,6 @@
 
 from fastapi import Depends, FastAPI, Request
 from labthings_fastapi.deps import InvocationID
-from labthings_fastapi.file_manager import FileManagerDep
 from fastapi.testclient import TestClient
 from module_with_deps import FancyIDDep
 
@@ -55,17 +54,3 @@ def endpoint(id: DepClass = Depends()) -> bool:
         assert r.status_code == 200
         invocation = r.json()
         assert invocation is True
-
-
-def test_file_manager():
-    app = FastAPI()
-
-    @app.post("/invoke_with_file")
-    def invoke_with_file(
-        file_manager: FileManagerDep,
-    ) -> dict[str, str]:
-        return {"directory": str(file_manager.directory)}
-
-    with TestClient(app) as client:
-        r = client.post("/invoke_with_file")
-        assert r.status_code == 200

tests/test_dependencies.py

@@ -22,7 +22,6 @@
 from fastapi.testclient import TestClient
 from module_with_deps import FancyIDDep, FancyID, ClassDependsOnFancyID
 import labthings_fastapi as lt
-from labthings_fastapi.file_manager import FileManager
 
 
 def test_dep_from_module():
@@ -147,17 +146,3 @@ def endpoint(id: lt.deps.InvocationID) -> bool:
     with TestClient(app) as client:
         r = client.post("/endpoint")
         assert r.status_code == 200
-
-
-def test_filemanager_dep():
-    """Test out our FileManager class as a dependency"""
-    app = FastAPI()
-
-    @app.post("/endpoint")
-    def endpoint(fm: Annotated[FileManager, Depends()]) -> str:
-        return f"Saving to {fm.directory}"
-
-    with TestClient(app) as client:
-        r = client.post("/endpoint")
-        assert r.status_code == 200
-        assert r.json().startswith("Saving to ")