Merge pull request #100 from opensafely-core/container-clean-ups

container clean ups

Author: bloodearnest

airlock/api.py

@@ -3,7 +3,10 @@
 from datetime import datetime
 from enum import Enum
 from pathlib import Path
-from typing import Optional
+
+# we use PurePosixPath as a convenient url path representation
+from pathlib import PurePosixPath as UrlPath
+from typing import Optional, Protocol
 
 from django.conf import settings
 from django.shortcuts import reverse
@@ -12,6 +15,9 @@
 from airlock.users import User
 
 
+ROOT_PATH = UrlPath()  # empty path
+
+
 class Status(Enum):
     """Status for release Requests"""
 
@@ -25,27 +31,30 @@ class Status(Enum):
     RELEASED = "RELEASED"
 
 
-class AirlockContainer:
-    """Abstract class for a directory or workspace or request files.
+class AirlockContainer(Protocol):
+    """Structural typing class for a instance of a Workspace or ReleaseRequest
 
-    Provides a uniform interface for accessing information about this directory.
+    Provides a uniform interface for accessing information about the paths and files
+    contained within this instance.
     """
 
-    def root(self):
-        raise NotImplementedError()
+    # The currently selected path in this container.  Used to calculate how
+    # a particular path relates to it, i.e. if path *is* the currently selected
+    # path, or is one of its parents.
+    selected_path: UrlPath = ROOT_PATH
 
-    def get_id(self):
-        raise NotImplementedError()
+    def root(self) -> Path:
+        """Absolute concrete Path to root dir for files in this container."""
 
-    def get_absolute_url(self):
-        raise NotImplementedError()
+    def get_id(self) -> str:
+        """Get the human name for this container."""
 
-    def get_url_for_path(self):
-        raise NotImplementedError()
+    def get_url(self, path: UrlPath = ROOT_PATH) -> str:
+        """Get the url for the container object with path"""
 
 
 @dataclass
-class Workspace(AirlockContainer):
+class Workspace:
     """Simple wrapper around a workspace directory on disk.
 
     Deliberately a dumb python object - the only operations are about accessing
@@ -54,6 +63,9 @@ class Workspace(AirlockContainer):
 
     name: str
 
+    # can be set to mark the currently selected path in this workspace
+    selected_path: UrlPath = ROOT_PATH
+
     def __post_init__(self):
         if not self.root().exists():
             raise ProviderAPI.WorkspaceNotFound(self.name)
@@ -64,10 +76,7 @@ def root(self):
     def get_id(self):
         return self.name
 
-    def get_absolute_url(self):
-        return reverse("workspace_home", kwargs={"workspace_name": self.name})
-
-    def get_url_for_path(self, relpath):
+    def get_url(self, relpath=ROOT_PATH):
         return reverse(
             "workspace_view",
             kwargs={"workspace_name": self.name, "path": relpath},
@@ -99,7 +108,7 @@ class RequestFile:
     Represents a single file within a release request
     """
 
-    relpath: Path
+    relpath: UrlPath
 
 
 @dataclass(frozen=True)
@@ -113,7 +122,7 @@ class FileGroup:
 
 
 @dataclass
-class ReleaseRequest(AirlockContainer):
+class ReleaseRequest:
     """Represents a release request made by a user.
 
     Deliberately a dumb python object. Does not operate on the state of request,
@@ -129,6 +138,9 @@ class ReleaseRequest(AirlockContainer):
     status: Status = Status.PENDING
     filegroups: dict[FileGroup] = field(default_factory=dict)
 
+    # can be set to mark the currently selected path in this release request
+    selected_path: UrlPath = ROOT_PATH
+
     def __post_init__(self):
         self.root().mkdir(parents=True, exist_ok=True)
 
@@ -138,15 +150,7 @@ def root(self):
     def get_id(self):
         return self.id
 
-    def get_absolute_url(self):
-        return reverse(
-            "request_home",
-            kwargs={
-                "request_id": self.id,
-            },
-        )
-
-    def get_url_for_path(self, relpath):
+    def get_url(self, relpath=ROOT_PATH):
         return reverse(
             "request_view",
             kwargs={
@@ -324,7 +328,7 @@ def set_status(
     def add_file_to_request(
         self,
         release_request: ReleaseRequest,
-        relpath: Path,
+        relpath: UrlPath,
         user: User,
         group_name: Optional[str] = "default",
     ):
@@ -370,7 +374,7 @@ def release_files(self, request: ReleaseRequest, user: User):
         )
 
         for f in filelist.files:
-            relpath = Path(f.name)
+            relpath = UrlPath(f.name)
             old_api.upload_file(
                 jobserver_release_id, relpath, request.root() / relpath, user.username
             )

airlock/file_browser_api.py

@@ -1,13 +1,12 @@
-import pathlib
 from dataclasses import dataclass
 
-from airlock.api import AirlockContainer
+from airlock.api import ROOT_PATH, AirlockContainer, UrlPath
 
 
 @dataclass(frozen=True)
 class PathItem:
     """
-    This provides a thin abstraction over `pathlib.Path` objects with two goals:
+    This provides a thin abstraction over url paths and related filesystem objects with two goals:
 
         1. Paths should be enforced as being relative to a certain "container" directory
            and it should not be possible to traverse outside of this directory or to
@@ -19,16 +18,11 @@ class PathItem:
     """
 
     container: AirlockContainer
-    relpath: pathlib.Path
-
-    # The currently selected view path in this container.  Used to calculate if
-    # how relpath relates to it, i.e. if relpath *is* the currently selected
-    # path, or is one of its parents.
-    selected: pathlib.Path = None
+    relpath: UrlPath = ROOT_PATH
 
     def __post_init__(self):
         # ensure relpath is a Path
-        object.__setattr__(self, "relpath", pathlib.Path(self.relpath))
+        object.__setattr__(self, "relpath", UrlPath(self.relpath))
         # ensure path is within container
         self._absolute_path().resolve().relative_to(self.container.root())
 
@@ -42,22 +36,25 @@ def is_directory(self):
         return self._absolute_path().is_dir()
 
     def name(self):
+        if self.relpath == ROOT_PATH:
+            return self.container.get_id()
+
         return self.relpath.name
 
     def url(self):
         suffix = "/" if self.is_directory() else ""
-        return self.container.get_url_for_path(f"{self.relpath}{suffix}")
+        return self.container.get_url(f"{self.relpath}{suffix}")
 
     def parent(self):
         if self.relpath.parents:
-            return PathItem(self.container, self.relpath.parent, selected=self.selected)
+            return PathItem(self.container, self.relpath.parent)
 
     def children(self):
         if not self.is_directory():
             return []
         root = self.container.root()
         children = [
-            PathItem(self.container, child.relative_to(root), selected=self.selected)
+            PathItem(self.container, child.relative_to(root))
             for child in self._absolute_path().iterdir()
         ]
         # directories first, then alphabetical, aks what windows does
@@ -85,8 +82,7 @@ def breadcrumbs(self):
 
         parent = item.parent()
         while parent:
-            if parent.relpath != pathlib.Path("."):
-                crumbs.append(parent)
+            crumbs.append(parent)
             parent = parent.parent()
 
         crumbs.reverse()
@@ -113,10 +109,10 @@ def html_classes(self):
         return " ".join(classes)
 
     def is_selected(self):
-        return self.relpath == self.selected
+        return self.relpath == self.container.selected_path
 
     def is_on_selected_path(self):
-        return self.relpath in self.selected.parents
+        return self.relpath in self.container.selected_path.parents
 
     def is_open(self):
         return self.is_selected() or self.is_on_selected_path()

airlock/templates/file_browser/index.html

@@ -71,7 +71,7 @@
             </form>
             {% endif %}
         {% endif %}
-        {% #button type="link" href=workspace.get_absolute_url variant="success" %}Workspace Home{% /button %}
+        {% #button type="link" href=workspace.get_url variant="success" %}Workspace Home{% /button %}
     {% endif %}
 </div>
 {% endfragment %}
@@ -84,7 +84,6 @@
 {% /card %}
 
 {% #breadcrumbs %}
-    {% breadcrumb title=path_item.container.get_id url=path_item.container.get_absolute_url %}
     {% for crumb in path_item.breadcrumbs %}
         {% breadcrumb title=crumb.name url=crumb.url active=forloop.last %}
     {% endfor %}

airlock/templates/requests.html

@@ -5,7 +5,7 @@
 {% #card title="Requests by "|add:request.user.username %}
   {% #list_group %}
     {% for request in authored_requests %} 
-    {% #list_group_item href=request.get_absolute_url %}{{ request.workspace }}: {{ request.status.name }}{% /list_group_item %}
+    {% #list_group_item href=request.get_url %}{{ request.workspace }}: {{ request.status.name }}{% /list_group_item %}
     {% endfor %}
   {% /list_group %}
 {% /card %}
@@ -14,7 +14,7 @@
 {% #card title="Outstanding requests awaiting review" %}
   {% #list_group %}
     {% for request in outstanding_requests %} 
-    {% #list_group_item href=request.get_absolute_url %}{{ request.workspace }} by {{ request.author }}{% /list_group_item %}
+    {% #list_group_item href=request.get_url %}{{ request.workspace }} by {{ request.author }}{% /list_group_item %}
     {% endfor %}
   {% /list_group %}
 {% /card %}

airlock/templates/workspaces.html

@@ -4,7 +4,7 @@
 {% #card title="Workspaces for "|add:request.user.username %}
   {% #list_group %}
     {% for workspace in workspaces %} 
-    {% #list_group_item href=workspace.get_absolute_url %}{{ workspace.name }}{% /list_group_item %}
+    {% #list_group_item href=workspace.get_url %}{{ workspace.name }}{% /list_group_item %}
     {% endfor %}
   {% /list_group %}
 {% /card %}

airlock/urls.py

@@ -34,7 +34,8 @@
     path(
         "workspaces/view/<str:workspace_name>/",
         airlock.views.workspace_view,
-        name="workspace_home",
+        kwargs={"path": ""},
+        name="workspace_view",
     ),
     path(
         "workspaces/view/<str:workspace_name>/<path:path>",
@@ -55,7 +56,8 @@
     path(
         "requests/view/<str:request_id>/",
         airlock.views.request_view,
-        name="request_home",
+        name="request_view",
+        kwargs={"path": ""},
     ),
     path(
         "requests/view/<str:request_id>/<path:path>",

airlock/views.py

@@ -1,5 +1,3 @@
-from pathlib import Path
-
 import requests
 from django import forms
 from django.conf import settings
@@ -12,7 +10,7 @@
 from django.views.decorators.http import require_http_methods
 
 from airlock import login_api
-from airlock.api import Status
+from airlock.api import Status, UrlPath
 from airlock.file_browser_api import PathItem
 from airlock.forms import AddFileForm
 from local_db.api import LocalDBProvider
@@ -132,9 +130,10 @@ def use_tree_ui(request):
 def workspace_view(request, workspace_name: str, path: str = ""):
     workspace = validate_workspace(request.user, workspace_name)
 
-    relpath = Path(path)
-    root = PathItem(workspace, Path("."), selected=relpath)
-    path_item = PathItem(workspace, path, selected=relpath)
+    relpath = UrlPath(path)
+    workspace.selected_path = relpath
+    root = PathItem(workspace)
+    path_item = PathItem(workspace, relpath)
 
     if not path_item.exists():
         raise Http404()
@@ -167,7 +166,7 @@ def workspace_view(request, workspace_name: str, path: str = ""):
 @require_http_methods(["POST"])
 def workspace_add_file_to_request(request, workspace_name):
     workspace = validate_workspace(request.user, workspace_name)
-    relpath = Path(request.POST["path"])
+    relpath = UrlPath(request.POST["path"])
     try:
         workspace.abspath(relpath)
     except api.FileNotFound:
@@ -178,7 +177,7 @@ def workspace_add_file_to_request(request, workspace_name):
     if not form.is_valid():
         for error in form.errors.values():
             messages.error(request, error)
-        return redirect(workspace.get_url_for_path(relpath))
+        return redirect(workspace.get_url(relpath))
 
     group_name = request.POST.get("new_filegroup") or request.POST.get("filegroup")
     try:
@@ -192,7 +191,7 @@ def workspace_add_file_to_request(request, workspace_name):
             request, f"File has been added to request (file group '{group_name}')"
         )
     # redirect to this just added file
-    return redirect(release_request.get_url_for_path(relpath))
+    return redirect(release_request.get_url(relpath))
 
 
 def request_index(request):
@@ -215,9 +214,10 @@ def request_index(request):
 def request_view(request, request_id: str, path: str = ""):
     release_request = validate_release_request(request.user, request_id)
 
-    relpath = Path(path)
-    root = PathItem(release_request, Path("."), selected=relpath)
-    path_item = PathItem(release_request, path, selected=relpath)
+    relpath = UrlPath(path)
+    release_request.selected_path = relpath
+    root = PathItem(release_request)
+    path_item = PathItem(release_request, relpath)
 
     if not path_item.exists():
         raise Http404()
@@ -273,7 +273,7 @@ def request_submit(request, request_id):
         raise PermissionDenied(str(exc))
 
     messages.success(request, "Request has been submitted")
-    return redirect(release_request.get_absolute_url())
+    return redirect(release_request.get_url())
 
 
 @require_http_methods(["POST"])
@@ -286,7 +286,7 @@ def request_reject(request, request_id):
         raise PermissionDenied(str(exc))
 
     messages.error(request, "Request has been rejected")
-    return redirect(release_request.get_absolute_url())
+    return redirect(release_request.get_url())
 
 
 @require_http_methods(["POST"])
@@ -315,4 +315,4 @@ def request_release_files(request, request_id):
         raise
 
     messages.success(request, "Files have been released to jobs.opensafely.org")
-    return redirect(release_request.get_absolute_url())
+    return redirect(release_request.get_url())