Merge pull request #155 from opensafely-core/evansd/api-refactor

Refactor ProviderAPI to Business Logic and Data Access Layers

airlock/api.py → airlock/business_logic.py

@@ -10,6 +10,8 @@
 
 from django.conf import settings
 from django.shortcuts import reverse
+from django.utils.functional import SimpleLazyObject
+from django.utils.module_loading import import_string
 
 import old_api
 from airlock.users import User
@@ -73,7 +75,7 @@ class Workspace:
 
     def __post_init__(self):
         if not self.root().exists():
-            raise ProviderAPI.WorkspaceNotFound(self.name)
+            raise BusinessLogicLayer.WorkspaceNotFound(self.name)
 
     def root(self):
         return settings.WORKSPACE_DIR / self.name
@@ -105,7 +107,7 @@ def abspath(self, relpath):
 
         # validate path exists
         if not path.exists():
-            raise ProviderAPI.FileNotFound(path)
+            raise BusinessLogicLayer.FileNotFound(path)
 
         return path
 
@@ -121,6 +123,10 @@ class RequestFile:
 
     relpath: UrlPath
 
+    @classmethod
+    def from_dict(cls, attrs):
+        return cls(**attrs)
+
 
 @dataclass(frozen=True)
 class FileGroup:
@@ -131,6 +137,13 @@ class FileGroup:
     name: str
     files: list[RequestFile]
 
+    @classmethod
+    def from_dict(cls, attrs):
+        return cls(
+            **{k: v for k, v in attrs.items() if k != "files"},
+            files=[RequestFile.from_dict(value) for value in attrs.get("files", ())],
+        )
+
 
 @dataclass
 class ReleaseRequest:
@@ -152,6 +165,17 @@ class ReleaseRequest:
     # can be set to mark the currently selected path in this release request
     selected_path: UrlPath = ROOT_PATH
 
+    @classmethod
+    def from_dict(cls, attrs):
+        return cls(
+            **{k: v for k, v in attrs.items() if k != "filegroups"},
+            filegroups=cls._filegroups_from_dict(attrs.get("filegroups", {})),
+        )
+
+    @staticmethod
+    def _filegroups_from_dict(attrs):
+        return {key: FileGroup.from_dict(value) for key, value in attrs.items()}
+
     def __post_init__(self):
         self.root().mkdir(parents=True, exist_ok=True)
 
@@ -190,7 +214,7 @@ def abspath(self, relpath):
         group = relpath.parts[0]
 
         if group not in self.filegroups:
-            raise ProviderAPI.FileNotFound(f"bad group {group} in url {relpath}")
+            raise BusinessLogicLayer.FileNotFound(f"bad group {group} in url {relpath}")
 
         filepath = relpath.relative_to(group)
         path = root / filepath
@@ -200,7 +224,7 @@ def abspath(self, relpath):
 
         # validate path exists
         if not path.exists():
-            raise ProviderAPI.FileNotFound(path)
+            raise BusinessLogicLayer.FileNotFound(path)
 
         return path
 
@@ -211,8 +235,27 @@ def file_set(self):
             for request_file in filegroup.files
         }
 
+    def set_filegroups_from_dict(self, attrs):
+        self.filegroups = self._filegroups_from_dict(attrs)
+
+
+class DataAccessLayerProtocol:
+    """
+    Placeholder for a structural type class we can use to define what a data access
+    layer should look like, once we've settled what that is.
+    """
+
+
+class BusinessLogicLayer:
+    """
+    The mechanism via which the rest of the codebase should read and write application
+    state. Interacts with a Data Access Layer purely by exchanging simple values
+    (dictionaries, strings etc).
+    """
+
+    def __init__(self, data_access_layer: DataAccessLayerProtocol):
+        self._dal = data_access_layer
 
-class ProviderAPI:
     class APIException(Exception):
         pass
 
@@ -265,11 +308,11 @@ def _create_release_request(self, **kwargs):
         Is private because it is mean to only be used by our test factories to
         set up state - it is not part of the public API.
         """
-        raise NotImplementedError()
+        return ReleaseRequest.from_dict(self._dal.create_release_request(**kwargs))
 
     def get_release_request(self, request_id: str) -> ReleaseRequest:
         """Get a ReleaseRequest object for an id."""
-        raise NotImplementedError()
+        return ReleaseRequest.from_dict(self._dal.get_release_request(request_id))
 
     def get_current_request(
         self, workspace_name: str, user: User, create: bool = False
@@ -278,15 +321,52 @@ def get_current_request(
 
         If create is True, create one.
         """
-        raise NotImplementedError()
+        active_requests = self._dal.get_active_requests_for_workspace_by_user(
+            workspace=workspace_name,
+            username=user.username,
+        )
+
+        n = len(active_requests)
+        if n > 1:
+            raise Exception(
+                f"Multiple active release requests for user {user.username} in workspace {workspace_name}"
+            )
+        elif n == 1:
+            return ReleaseRequest.from_dict(active_requests[0])
+        elif create:
+            # To create a request, you must have explicit workspace permissions.
+            # Output checkers can view all workspaces, but are not allowed to
+            # create requests for all workspaces.
+            if workspace_name not in user.workspaces:
+                raise BusinessLogicLayer.RequestPermissionDenied(workspace_name)
+
+            new_request = self._dal.create_release_request(
+                workspace=workspace_name,
+                author=user.username,
+            )
+            return ReleaseRequest.from_dict(new_request)
+        else:
+            return None
 
     def get_requests_authored_by_user(self, user: User) -> list[ReleaseRequest]:
         """Get all current requests authored by user."""
-        raise NotImplementedError()
+        return [
+            ReleaseRequest.from_dict(attrs)
+            for attrs in self._dal.get_requests_authored_by_user(username=user.username)
+        ]
 
     def get_outstanding_requests_for_review(self, user: User):
         """Get all request that need review."""
-        raise NotImplementedError()
+        # Only output checkers can see these
+        if not user.output_checker:
+            return []
+
+        return [
+            ReleaseRequest.from_dict(attrs)
+            for attrs in self._dal.get_outstanding_requests_for_review()
+            # Do not show output_checker their own requests
+            if attrs["author"] != user.username
+        ]
 
     VALID_STATE_TRANSITIONS = {
         Status.PENDING: [
@@ -364,6 +444,7 @@ def set_status(
 
         # validate first
         self.check_status(release_request, to_status, user)
+        self._dal.set_status(release_request.id, to_status)
         release_request.status = to_status
 
     def add_file_to_request(
@@ -373,14 +454,6 @@ def add_file_to_request(
         user: User,
         group_name: Optional[str] = "default",
     ):
-        """Add a file to a request.
-
-        Subclasses should do what they need to create the filegroup and
-        record the file metadata as needed and THEN
-        call super().add_file_to_request(...) to do the
-        copying. If the copying fails (e.g. due to permission errors raised
-        below), the subclasses should roll back any changes.
-        """
         if user.username != release_request.author:
             raise self.RequestPermissionDenied(
                 f"only author {release_request.author} can add files to this request"
@@ -398,6 +471,14 @@ def add_file_to_request(
         dst.parent.mkdir(exist_ok=True, parents=True)
         shutil.copy(src, dst)
 
+        # TODO: This is not currently safe in that we modify the filesytem before
+        # calling out to the DAL which could fail. We will deal with this later by
+        # switching to a content-addressed storage model which avoids having mutable
+        # state on the filesystem.
+        filegroup_data = self._dal.add_file_to_request(
+            request_id=release_request.id, group_name=group_name, relpath=relpath
+        )
+        release_request.set_filegroups_from_dict(filegroup_data)
         return release_request
 
     def release_files(self, request: ReleaseRequest, user: User):
@@ -422,3 +503,13 @@ def release_files(self, request: ReleaseRequest, user: User):
             )
 
         self.set_status(request, Status.RELEASED, user)
+
+
+def _get_configured_bll():
+    DataAccessLayer = import_string(settings.AIRLOCK_DATA_ACCESS_LAYER)
+    return BusinessLogicLayer(DataAccessLayer())
+
+
+# We follow the Django pattern of using a lazy object which configures itself on first
+# access so as to avoid reading `settings` during import
+bll = SimpleLazyObject(_get_configured_bll)

airlock/file_browser_api.py

@@ -1,7 +1,7 @@
 from dataclasses import dataclass, field
 from enum import Enum
 
-from airlock.api import ROOT_PATH, AirlockContainer, UrlPath
+from airlock.business_logic import ROOT_PATH, AirlockContainer, UrlPath
 
 
 class PathType(Enum):

airlock/settings.py

@@ -268,6 +268,8 @@ def run_connection_init_queries(*, connection, **kwargs):
         f"variables must be set.\n\n{_missing_env_var_hint}"
     )
 
+AIRLOCK_DATA_ACCESS_LAYER = "local_db.data_access.LocalDBDataAccessLayer"
+
 # BACKEND is global env var on backends
 BACKEND = os.environ.get("BACKEND", "test")
 

airlock/views/helpers.py

@@ -3,10 +3,7 @@
 from django.core.exceptions import PermissionDenied
 from django.http import FileResponse, Http404
 
-from local_db.api import LocalDBProvider
-
-
-api = LocalDBProvider()
+from airlock.business_logic import bll
 
 
 def login_exempt(view):
@@ -17,8 +14,8 @@ def login_exempt(view):
 def validate_workspace(user, workspace_name):
     """Ensure the workspace exists and the current user has permissions to access it."""
     try:
-        workspace = api.get_workspace(workspace_name)
-    except api.WorkspaceNotFound:
+        workspace = bll.get_workspace(workspace_name)
+    except bll.WorkspaceNotFound:
         raise Http404()
 
     if user is None or not user.has_permission(workspace_name):
@@ -30,8 +27,8 @@ def validate_workspace(user, workspace_name):
 def validate_release_request(user, request_id):
     """Ensure the release request exists for this workspace."""
     try:
-        release_request = api.get_release_request(request_id)
-    except api.ReleaseRequestNotFound:
+        release_request = bll.get_release_request(request_id)
+    except bll.ReleaseRequestNotFound:
         raise Http404()
 
     # check user permissions for this workspace

airlock/views/request.py

@@ -9,22 +9,18 @@
 from django.views.decorators.http import require_http_methods
 from django.views.decorators.vary import vary_on_headers
 
-from airlock.api import Status
+from airlock.business_logic import Status, bll
 from airlock.file_browser_api import get_request_tree
-from local_db.api import LocalDBProvider
 
 from .helpers import serve_file, validate_release_request
 
 
-api = LocalDBProvider()
-
-
 def request_index(request):
-    authored_requests = api.get_requests_authored_by_user(request.user)
+    authored_requests = bll.get_requests_authored_by_user(request.user)
 
     outstanding_requests = []
     if request.user.output_checker:
-        outstanding_requests = api.get_outstanding_requests_for_review(request.user)
+        outstanding_requests = bll.get_outstanding_requests_for_review(request.user)
 
     return TemplateResponse(
         request,
@@ -78,7 +74,7 @@ def request_view(request, request_id: str, path: str = ""):
     )
 
     context = {
-        "workspace": api.get_workspace(release_request.workspace),
+        "workspace": bll.get_workspace(release_request.workspace),
         "release_request": release_request,
         "root": tree,
         "path_item": path_item,
@@ -101,7 +97,7 @@ def request_contents(request, request_id: str, path: str):
 
     try:
         abspath = release_request.abspath(path)
-    except api.FileNotFound:
+    except bll.FileNotFound:
         raise Http404()
 
     if not abspath.is_file():
@@ -125,8 +121,8 @@ def request_submit(request, request_id):
     release_request = validate_release_request(request.user, request_id)
 
     try:
-        api.set_status(release_request, Status.SUBMITTED, request.user)
-    except api.RequestPermissionDenied as exc:
+        bll.set_status(release_request, Status.SUBMITTED, request.user)
+    except bll.RequestPermissionDenied as exc:
         raise PermissionDenied(str(exc))
 
     messages.success(request, "Request has been submitted")
@@ -138,8 +134,8 @@ def request_reject(request, request_id):
     release_request = validate_release_request(request.user, request_id)
 
     try:
-        api.set_status(release_request, Status.REJECTED, request.user)
-    except api.RequestPermissionDenied as exc:
+        bll.set_status(release_request, Status.REJECTED, request.user)
+    except bll.RequestPermissionDenied as exc:
         raise PermissionDenied(str(exc))
 
     messages.error(request, "Request has been rejected")
@@ -152,9 +148,9 @@ def request_release_files(request, request_id):
 
     try:
         # For now, we just implicitly approve when release files is requested
-        api.set_status(release_request, Status.APPROVED, request.user)
-        api.release_files(release_request, request.user)
-    except api.RequestPermissionDenied as exc:
+        bll.set_status(release_request, Status.APPROVED, request.user)
+        bll.release_files(release_request, request.user)
+    except bll.RequestPermissionDenied as exc:
         raise PermissionDenied(str(exc))
     except requests.HTTPError as err:
         if settings.DEBUG: