diff --git a/airlock/api.py b/airlock/business_logic.py similarity index 73% rename from airlock/api.py rename to airlock/business_logic.py index 208c90ee..ddeb9276 100644 --- a/airlock/api.py +++ b/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) diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py index 9ca4e4c4..c65046d5 100644 --- a/airlock/file_browser_api.py +++ b/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): diff --git a/airlock/settings.py b/airlock/settings.py index 185cca7c..a4ad8411 100644 --- a/airlock/settings.py +++ b/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") diff --git a/airlock/views/helpers.py b/airlock/views/helpers.py index 91300c12..a69998c7 100644 --- a/airlock/views/helpers.py +++ b/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 diff --git a/airlock/views/request.py b/airlock/views/request.py index d14a5691..c1713cdc 100644 --- a/airlock/views/request.py +++ b/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: diff --git a/airlock/views/workspace.py b/airlock/views/workspace.py index 665db26f..ffc129c6 100644 --- a/airlock/views/workspace.py +++ b/airlock/views/workspace.py @@ -6,19 +6,15 @@ from django.views.decorators.http import require_http_methods from django.views.decorators.vary import vary_on_headers -from airlock.api import UrlPath +from airlock.business_logic import UrlPath, bll from airlock.file_browser_api import get_workspace_tree from airlock.forms import AddFileForm -from local_db.api import LocalDBProvider from .helpers import serve_file, validate_workspace -api = LocalDBProvider() - - def workspace_index(request): - workspaces = api.get_workspaces_for_user(request.user) + workspaces = bll.get_workspaces_for_user(request.user) return TemplateResponse(request, "workspaces.html", {"workspaces": workspaces}) @@ -45,7 +41,7 @@ def workspace_view(request, workspace_name: str, path: str = ""): if path_item.is_directory() != is_directory_url: return redirect(path_item.url()) - current_request = api.get_current_request(workspace_name, request.user) + current_request = bll.get_current_request(workspace_name, request.user) # Only include the AddFileForm if this pathitem is a file that # can be added to a request - i.e. it is a file and it's not @@ -92,7 +88,7 @@ def workspace_contents(request, workspace_name: str, path: str): try: abspath = workspace.abspath(path) - except api.FileNotFound: + except bll.FileNotFound: raise Http404() if not abspath.is_file(): @@ -107,16 +103,16 @@ def workspace_add_file_to_request(request, workspace_name): relpath = UrlPath(request.POST["path"]) try: workspace.abspath(relpath) - except api.FileNotFound: + except bll.FileNotFound: raise Http404() - release_request = api.get_current_request(workspace_name, request.user, create=True) + release_request = bll.get_current_request(workspace_name, request.user, create=True) form = AddFileForm(request.POST, release_request=release_request) if form.is_valid(): group_name = request.POST.get("new_filegroup") or request.POST.get("filegroup") try: - api.add_file_to_request(release_request, relpath, request.user, group_name) - except api.APIException as err: + bll.add_file_to_request(release_request, relpath, request.user, group_name) + except bll.APIException as err: # This exception is raised if the file has already been added # (to any group on the request) messages.error(request, str(err)) diff --git a/local_db/api.py b/local_db/api.py deleted file mode 100644 index 7a0cab9d..00000000 --- a/local_db/api.py +++ /dev/null @@ -1,168 +0,0 @@ -from dataclasses import dataclass -from pathlib import Path -from typing import Optional - -from django.db import transaction - -from airlock.api import ( - FileGroup, - ProviderAPI, - ReleaseRequest, - RequestFile, - Status, - User, -) -from local_db.models import FileGroupMetadata, RequestFileMetadata, RequestMetadata - - -@dataclass -class LocalDBProvider(ProviderAPI): - """Implementation of ProviderAPI using local_db models to store data.""" - - def _request(self, metadata: RequestMetadata = None): - """Unpack the db data into the Request object.""" - return ReleaseRequest( - id=metadata.id, - workspace=metadata.workspace, - status=metadata.status, - author=metadata.author, - created_at=metadata.created_at, - filegroups=self._get_filegroups(metadata), - ) - - def _create_release_request(self, **kwargs): - metadata = RequestMetadata.objects.create(**kwargs) - return self._request(metadata) - - def _find_metadata(self, request_id: str): - try: - return RequestMetadata.objects.get(id=request_id) - except RequestMetadata.DoesNotExist: - raise self.ReleaseRequestNotFound(request_id) - - def _filegroup(self, filegroup_metadata: FileGroupMetadata): - """Unpack file group db data into FileGroup and RequestFile objects.""" - return FileGroup( - name=filegroup_metadata.name, - files=[ - RequestFile(relpath=Path(file_metadata.relpath)) - for file_metadata in filegroup_metadata.request_files.all() - ], - ) - - def _get_filegroups(self, metadata: RequestMetadata): - return { - group_metadata.name: self._filegroup(group_metadata) - for group_metadata in metadata.filegroups.all() - } - - def _get_or_create_filegroupmetadata(self, request_id: str, group_name: str): - metadata = self._find_metadata(request_id) - groupmetadata, _ = FileGroupMetadata.objects.get_or_create( - request=metadata, name=group_name - ) - return groupmetadata - - def get_release_request(self, request_id: str): - return self._request(self._find_metadata(request_id)) - - def get_current_request(self, workspace: str, user: User, create=False): - requests = list( - RequestMetadata.objects.filter( - workspace=workspace, - author=user.username, - status__in=[Status.PENDING, Status.SUBMITTED], - ) - ) - n = len(requests) - if n > 1: - raise Exception( - f"Multiple active release requests for user {user.username} in workspace {workspace}" - ) - elif n == 1: - return self._request(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 not in user.workspaces: - raise self.RequestPermissionDenied(workspace) - - return self._create_release_request( - workspace=workspace, - author=user.username, - ) - - def get_requests_authored_by_user(self, user: User): - requests = [] - - for metadata in RequestMetadata.objects.filter(author=user.username).order_by( - "status" - ): - # to create a request, user *must* have explicit workspace - # permissions - being an output checker is not enough - if metadata.workspace in user.workspaces: - requests.append(self._request(metadata)) - - return requests - - def get_outstanding_requests_for_review(self, user: User): - requests = [] - - if not user.output_checker: - return [] - - for metadata in RequestMetadata.objects.filter(status=Status.SUBMITTED): - # do not show output_checker their own requests - if metadata.author != user.username: - requests.append(self._request(metadata)) - - return requests - - def set_status(self, request: ReleaseRequest, status: Status, user: User): - with transaction.atomic(): - # validate transition/permissions ahead of time - self.check_status(request, status, user) - # persist state change - metadata = self._find_metadata(request.id) - metadata.status = status - metadata.save() - super().set_status(request, status, user) - - def add_file_to_request( - self, - release_request: ReleaseRequest, - relpath: Path, - user: User, - group_name: Optional[str] = "default", - ): - with transaction.atomic(): - # Get/create the FileGroupMetadata if it doesn't already exist - filegroupmetadata = self._get_or_create_filegroupmetadata( - release_request.id, group_name - ) - # Check if this file is already on the request, in any group - try: - existing_file = RequestFileMetadata.objects.get( - filegroup__request_id=release_request.id, relpath=relpath - ) - except RequestFileMetadata.DoesNotExist: - # create the RequestFile - RequestFileMetadata.objects.create( - relpath=str(relpath), filegroup=filegroupmetadata - ) - else: - raise self.APIException( - "File has already been added to request " - f"(in file group '{existing_file.filegroup.name}')" - ) - - # Now that the db objects have been successfully creates, - # call super() to copy the file. This will also validate - # that the user can add the file, and that the request is in a - # state that allows adding files. - super().add_file_to_request(release_request, relpath, user, group_name) - - # update instance - metadata = self._find_metadata(release_request.id) - release_request.filegroups = self._get_filegroups(metadata) diff --git a/local_db/data_access.py b/local_db/data_access.py new file mode 100644 index 00000000..2aed2908 --- /dev/null +++ b/local_db/data_access.py @@ -0,0 +1,120 @@ +from pathlib import Path + +from django.db import transaction + +from airlock.business_logic import ( + BusinessLogicLayer, + DataAccessLayerProtocol, + Status, +) +from local_db.models import FileGroupMetadata, RequestFileMetadata, RequestMetadata + + +class LocalDBDataAccessLayer(DataAccessLayerProtocol): + """ + Implementation of DataAccessLayerProtocol using local_db models to store data + """ + + def _request(self, metadata: RequestMetadata = None): + """Unpack the db data into the Request object.""" + return dict( + id=metadata.id, + workspace=metadata.workspace, + status=metadata.status, + author=metadata.author, + created_at=metadata.created_at, + filegroups=self._get_filegroups(metadata), + ) + + def create_release_request(self, **kwargs): + metadata = RequestMetadata.objects.create(**kwargs) + return self._request(metadata) + + def _find_metadata(self, request_id: str): + try: + return RequestMetadata.objects.get(id=request_id) + except RequestMetadata.DoesNotExist: + raise BusinessLogicLayer.ReleaseRequestNotFound(request_id) + + def _filegroup(self, filegroup_metadata: FileGroupMetadata): + """Unpack file group db data into FileGroup and RequestFile objects.""" + return dict( + name=filegroup_metadata.name, + files=[ + dict(relpath=Path(file_metadata.relpath)) + for file_metadata in filegroup_metadata.request_files.all() + ], + ) + + def _get_filegroups(self, metadata: RequestMetadata): + return { + group_metadata.name: self._filegroup(group_metadata) + for group_metadata in metadata.filegroups.all() + } + + def _get_or_create_filegroupmetadata(self, request_id: str, group_name: str): + metadata = self._find_metadata(request_id) + groupmetadata, _ = FileGroupMetadata.objects.get_or_create( + request=metadata, name=group_name + ) + return groupmetadata + + def get_release_request(self, request_id: str): + return self._request(self._find_metadata(request_id)) + + def get_active_requests_for_workspace_by_user(self, workspace: str, username: str): + return [ + self._request(request) + for request in RequestMetadata.objects.filter( + workspace=workspace, + author=username, + status__in=[Status.PENDING, Status.SUBMITTED], + ) + ] + + def get_requests_authored_by_user(self, username: str): + return [ + self._request(request) + for request in RequestMetadata.objects.filter(author=username).order_by( + "status" + ) + ] + + def get_outstanding_requests_for_review(self): + return [ + self._request(metadata) + for metadata in RequestMetadata.objects.filter(status=Status.SUBMITTED) + ] + + def set_status(self, request_id: str, status: Status): + with transaction.atomic(): + # persist state change + metadata = self._find_metadata(request_id) + metadata.status = status + metadata.save() + + def add_file_to_request(self, request_id, relpath: Path, group_name: str): + with transaction.atomic(): + # Get/create the FileGroupMetadata if it doesn't already exist + filegroupmetadata = self._get_or_create_filegroupmetadata( + request_id, group_name + ) + # Check if this file is already on the request, in any group + try: + existing_file = RequestFileMetadata.objects.get( + filegroup__request_id=request_id, relpath=relpath + ) + except RequestFileMetadata.DoesNotExist: + # create the RequestFile + RequestFileMetadata.objects.create( + relpath=str(relpath), filegroup=filegroupmetadata + ) + else: + raise BusinessLogicLayer.APIException( + "File has already been added to request " + f"(in file group '{existing_file.filegroup.name}')" + ) + + # Return updated FileGroups data + metadata = self._find_metadata(request_id) + return self._get_filegroups(metadata) diff --git a/local_db/migrations/0002_requestmetadata_status.py b/local_db/migrations/0002_requestmetadata_status.py index 239f4730..b5b14c1f 100644 --- a/local_db/migrations/0002_requestmetadata_status.py +++ b/local_db/migrations/0002_requestmetadata_status.py @@ -2,7 +2,7 @@ from django.db import migrations -import airlock.api +import airlock.business_logic import local_db.models @@ -15,6 +15,8 @@ class Migration(migrations.Migration): migrations.AddField( model_name="requestmetadata", name="status", - field=local_db.models.StatusField(default=airlock.api.Status["PENDING"]), + field=local_db.models.StatusField( + default=airlock.business_logic.Status["PENDING"] + ), ), ] diff --git a/local_db/models.py b/local_db/models.py index 4f846487..2e8c8149 100644 --- a/local_db/models.py +++ b/local_db/models.py @@ -2,7 +2,7 @@ from django.utils import timezone from ulid import ulid -from airlock.api import Status +from airlock.business_logic import Status def local_request_id(): diff --git a/tests/conftest.py b/tests/conftest.py index 127ad102..2de6899a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,9 +2,9 @@ import responses as _responses from django.conf import settings +import airlock.business_logic import old_api import tests.factories -from local_db.api import LocalDBProvider # Fail the test run if we see any warnings @@ -31,12 +31,12 @@ def responses(): yield rsps -# we could parameterise this fixture to run tests over all api implementations in future +# We could parameterise this fixture to run tests over all Data Access Layer +# implementations in future @pytest.fixture -def api(monkeypatch): - api = LocalDBProvider() - monkeypatch.setattr(tests.factories, "api", api) - return api +def bll(monkeypatch): + monkeypatch.setattr(tests.factories, "bll", airlock.business_logic.bll) + return airlock.business_logic.bll @pytest.fixture diff --git a/tests/factories.py b/tests/factories.py index 478a3c70..40ffe874 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -1,14 +1,11 @@ from django.conf import settings -from airlock.api import Workspace +from airlock.business_logic import Workspace, bll from airlock.users import User -from local_db.api import LocalDBProvider default_user = User(1, "testuser") -api = LocalDBProvider() - def get_user(*, username="testuser", workspaces=[], output_checker=False): return User(1, username, workspaces, output_checker) @@ -26,7 +23,7 @@ def ensure_workspace(workspace_or_name): def create_workspace(name): workspace_dir = settings.WORKSPACE_DIR / name workspace_dir.mkdir(exist_ok=True, parents=True) - return api.get_workspace(name) + return bll.get_workspace(name) def write_workspace_file(workspace, path, contents=""): @@ -43,7 +40,7 @@ def create_release_request(workspace, user=None, **kwargs): if user is None: user = get_user(workspaces=[workspace.name]) - release_request = api._create_release_request( + release_request = bll._create_release_request( workspace=workspace.name, author=user.username, **kwargs ) release_request.root().mkdir(parents=True, exist_ok=True) @@ -54,23 +51,23 @@ def write_request_file(request, group, path, contents="", user=None): workspace = ensure_workspace(request.workspace) try: workspace.abspath(path) - except api.FileNotFound: + except bll.FileNotFound: write_workspace_file(workspace, path, contents) # create a default user with permission on workspace if user is None: # pragma: nocover user = get_user(username=request.author, workspaces=[request.workspace]) - api.add_file_to_request(request, relpath=path, user=user, group_name=group) + bll.add_file_to_request(request, relpath=path, user=user, group_name=group) def create_filegroup(release_request, group_name, filepaths=None): for filepath in filepaths or []: # pragma: nocover - api.add_file_to_request( + bll.add_file_to_request( release_request, filepath, User(1, release_request.author), group_name ) - return api._get_or_create_filegroupmetadata(release_request.id, group_name) + return bll._dal._get_or_create_filegroupmetadata(release_request.id, group_name) def refresh_release_request(release_request): - return api.get_release_request(release_request.id) + return bll.get_release_request(release_request.id) diff --git a/tests/functional/test_e2e.py b/tests/functional/test_e2e.py index 6f538c3b..a0120c4c 100644 --- a/tests/functional/test_e2e.py +++ b/tests/functional/test_e2e.py @@ -4,14 +4,10 @@ import pytest from playwright.sync_api import expect -from airlock.api import Status -from local_db.api import LocalDBProvider +from airlock.business_logic import Status, bll from tests import factories -api = LocalDBProvider() - - @pytest.fixture def dev_users(tmp_path, settings): settings.AIRLOCK_DEV_USERS_FILE = tmp_path / "dev_users.json" @@ -196,7 +192,7 @@ def test_e2e_release_files(page, live_server, dev_users, release_files_stubber): assert download.suggested_filename == "file.txt" # Mock the responses from job-server - release_request = api.get_release_request(request_id) + release_request = bll.get_release_request(request_id) release_files_stubber(release_request) # Release the files diff --git a/tests/integration/views/test_request.py b/tests/integration/views/test_request.py index e31d4f98..73c218a6 100644 --- a/tests/integration/views/test_request.py +++ b/tests/integration/views/test_request.py @@ -3,7 +3,7 @@ import pytest import requests -from airlock.api import Status +from airlock.business_logic import Status from airlock.users import User from tests import factories @@ -304,7 +304,7 @@ def test_request_submit_author(client_with_user): response = permitted_client.post(f"/requests/submit/{release_request.id}") assert response.status_code == 302 - persisted_request = factories.api.get_release_request(release_request.id) + persisted_request = factories.bll.get_release_request(release_request.id) assert persisted_request.status == Status.SUBMITTED @@ -319,7 +319,7 @@ def test_request_submit_not_author(client_with_user): response = permitted_client.post(f"/requests/submit/{release_request.id}") assert response.status_code == 403 - persisted_request = factories.api.get_release_request(release_request.id) + persisted_request = factories.bll.get_release_request(release_request.id) assert persisted_request.status == Status.PENDING @@ -335,7 +335,7 @@ def test_request_reject_output_checker(client_with_permission): response = client_with_permission.post(f"/requests/reject/{release_request.id}") assert response.status_code == 302 - persisted_request = factories.api.get_release_request(release_request.id) + persisted_request = factories.bll.get_release_request(release_request.id) assert persisted_request.status == Status.REJECTED @@ -352,7 +352,7 @@ def test_request_reject_not_output_checker(client_with_user): response = client.post(f"/requests/reject/{release_request.id}") assert response.status_code == 403 - persisted_request = factories.api.get_release_request(release_request.id) + persisted_request = factories.bll.get_release_request(release_request.id) assert persisted_request.status == Status.SUBMITTED diff --git a/tests/integration/views/test_workspace.py b/tests/integration/views/test_workspace.py index 4ee98369..12e53d4f 100644 --- a/tests/integration/views/test_workspace.py +++ b/tests/integration/views/test_workspace.py @@ -233,28 +233,28 @@ def test_workspaces_index_user_permitted_workspaces(client_with_user): assert "test2" not in response.rendered_content -def test_workspace_request_file_creates(client_with_user, api): +def test_workspace_request_file_creates(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) workspace = factories.create_workspace("test1") factories.write_workspace_file(workspace, "test/path.txt") - assert api.get_current_request(workspace.name, user) is None + assert bll.get_current_request(workspace.name, user) is None response = client.post( "/workspaces/add-file-to-request/test1", data={"path": "test/path.txt", "filegroup": "default"}, ) assert response.status_code == 302 - release_request = api.get_current_request(workspace.name, user) + release_request = bll.get_current_request(workspace.name, user) filegroup = release_request.filegroups["default"] assert filegroup.name == "default" assert str(filegroup.files[0].relpath) == "test/path.txt" assert release_request.abspath("default/test/path.txt").exists() -def test_workspace_request_file_request_already_exists(client_with_user, api): +def test_workspace_request_file_request_already_exists(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) @@ -268,7 +268,7 @@ def test_workspace_request_file_request_already_exists(client_with_user, api): data={"path": "test/path.txt", "filegroup": "default"}, ) assert response.status_code == 302 - current_release_request = api.get_current_request(workspace.name, user) + current_release_request = bll.get_current_request(workspace.name, user) assert current_release_request.id == release_request.id assert current_release_request.abspath("default/test/path.txt").exists() filegroup = current_release_request.filegroups["default"] @@ -276,14 +276,14 @@ def test_workspace_request_file_request_already_exists(client_with_user, api): assert str(filegroup.files[0].relpath) == "test/path.txt" -def test_workspace_request_file_with_new_filegroup(client_with_user, api): +def test_workspace_request_file_with_new_filegroup(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) workspace = factories.create_workspace("test1") factories.write_workspace_file(workspace, "test/path.txt") - assert api.get_current_request(workspace.name, user) is None + assert bll.get_current_request(workspace.name, user) is None response = client.post( "/workspaces/add-file-to-request/test1", data={ @@ -295,12 +295,12 @@ def test_workspace_request_file_with_new_filegroup(client_with_user, api): ) assert response.status_code == 302 - release_request = api.get_current_request(workspace.name, user) + release_request = bll.get_current_request(workspace.name, user) filegroup = release_request.filegroups["new_group"] assert filegroup.name == "new_group" -def test_workspace_request_file_filegroup_already_exists(client_with_user, api): +def test_workspace_request_file_filegroup_already_exists(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) @@ -342,7 +342,7 @@ def test_workspace_request_file_request_path_does_not_exist(client_with_user): assert response.status_code == 404 -def test_workspace_request_file_invalid_new_filegroup(client_with_user, api): +def test_workspace_request_file_invalid_new_filegroup(client_with_user, bll): client = client_with_user({"workspaces": ["test1"]}) user = User.from_session(client.session) diff --git a/tests/unit/test_api.py b/tests/unit/test_business_logic.py similarity index 72% rename from tests/unit/test_api.py rename to tests/unit/test_business_logic.py index 21f30ab7..91651643 100644 --- a/tests/unit/test_api.py +++ b/tests/unit/test_business_logic.py @@ -1,12 +1,12 @@ import json from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import MagicMock, Mock import pytest from django.conf import settings import old_api -from airlock.api import ProviderAPI, Status, UrlPath, Workspace +from airlock.business_logic import BusinessLogicLayer, Status, UrlPath, Workspace from airlock.users import User from tests import factories @@ -56,9 +56,9 @@ def test_provider_get_workspaces_for_user(user_workspaces, output_checker, expec factories.create_workspace("not-allowed") user = User(1, "test", user_workspaces, output_checker) - api = ProviderAPI() + bll = BusinessLogicLayer(data_access_layer=None) - assert set(api.get_workspaces_for_user(user)) == set(Workspace(w) for w in expected) + assert set(bll.get_workspaces_for_user(user)) == set(Workspace(w) for w in expected) @pytest.fixture @@ -79,9 +79,9 @@ def test_provider_request_release_files_not_approved(): status=Status.SUBMITTED, ) - api = ProviderAPI() - with pytest.raises(api.InvalidStateTransition): - api.release_files(release_request, checker) + bll = BusinessLogicLayer(data_access_layer=None) + with pytest.raises(bll.InvalidStateTransition): + bll.release_files(release_request, checker) def test_provider_request_release_files(mock_old_api): @@ -96,12 +96,12 @@ def test_provider_request_release_files(mock_old_api): ) relpath = Path("test/file.txt") factories.write_request_file(release_request, "group", relpath, "test") - factories.api.set_status(release_request, Status.APPROVED, checker) + factories.bll.set_status(release_request, Status.APPROVED, checker) abspath = release_request.abspath("group" / relpath) - api = ProviderAPI() - api.release_files(release_request, checker) + bll = BusinessLogicLayer(data_access_layer=Mock()) + bll.release_files(release_request, checker) expected_json = { "files": [ @@ -127,30 +127,13 @@ def test_provider_request_release_files(mock_old_api): ) -@pytest.mark.parametrize( - "workspaces, output_checker, expected", - [ - ([], False, []), - (["allowed"], False, ["r1"]), - # output checkers can't create requests unless explit permission for workspace - ([], True, []), - (["allowed"], True, ["r1"]), - (["allowed", "notexist"], False, ["r1"]), - (["notexist"], False, []), - (["no-request-dir", "notexist"], False, []), - ], -) -def test_provider_get_requests_authored_by_user( - workspaces, output_checker, expected, api -): - user = User(1, "test", workspaces, output_checker) - other_user = User(1, "other", [], False) - factories.create_release_request("allowed", user, id="r1") - factories.create_release_request("allowed", other_user, id="r2") - factories.create_release_request("not-allowed", user, id="r3") - factories.create_workspace("no-request-dir") +def test_provider_get_requests_authored_by_user(bll): + user = User(1, "test", [], True) + other_user = User(1, "other", [], True) + factories.create_release_request("workspace", user, id="r1") + factories.create_release_request("workspace", other_user, id="r2") - assert set(r.id for r in api.get_requests_authored_by_user(user)) == set(expected) + assert [r.id for r in bll.get_requests_authored_by_user(user)] == ["r1"] @pytest.mark.parametrize( @@ -163,7 +146,7 @@ def test_provider_get_requests_authored_by_user( (True, ["r1"]), ], ) -def test_provider_get_outstanding_requests_for_review(output_checker, expected, api): +def test_provider_get_outstanding_requests_for_review(output_checker, expected, bll): user = User(1, "test", ["workspace"], output_checker) other_user = User(1, "other", ["workspace"], False) # request created by another user, status submitted @@ -189,39 +172,39 @@ def test_provider_get_outstanding_requests_for_review(output_checker, expected, ws = f"workspace{i}" factories.create_release_request(ws, User(1, f"test_{i}", [ws]), status=status) - assert set(r.id for r in api.get_outstanding_requests_for_review(user)) == set( + assert set(r.id for r in bll.get_outstanding_requests_for_review(user)) == set( expected ) -def test_provider_get_current_request_for_user(api): +def test_provider_get_current_request_for_user(bll): workspace = factories.create_workspace("workspace") user = User(1, "testuser", ["workspace"], False) other_user = User(2, "otheruser", ["workspace"], False) - assert api.get_current_request("workspace", user) is None + assert bll.get_current_request("workspace", user) is None factories.create_release_request(workspace, other_user) - assert api.get_current_request("workspace", user) is None + assert bll.get_current_request("workspace", user) is None - release_request = api.get_current_request("workspace", user, create=True) + release_request = bll.get_current_request("workspace", user, create=True) assert release_request.workspace == "workspace" assert release_request.author == user.username # reach around an simulate 2 active requests for same user - api._create_release_request(author=user.username, workspace="workspace") + bll._create_release_request(author=user.username, workspace="workspace") with pytest.raises(Exception): - api.get_current_request("workspace", user) + bll.get_current_request("workspace", user) -def test_provider_get_current_request_for_user_output_checker(api): +def test_provider_get_current_request_for_user_output_checker(bll): """Output checker must have explict workspace permissions to create requests.""" factories.create_workspace("workspace") user = User(1, "output_checker", [], True) - with pytest.raises(api.RequestPermissionDenied): - api.get_current_request("workspace", user, create=True) + with pytest.raises(bll.RequestPermissionDenied): + bll.get_current_request("workspace", user, create=True) @pytest.mark.parametrize( @@ -252,7 +235,7 @@ def test_provider_get_current_request_for_user_output_checker(api): (Status.RELEASED, Status.WITHDRAWN, False, False), ], ) -def test_set_status(current, future, valid_author, valid_checker, api): +def test_set_status(current, future, valid_author, valid_checker, bll): author = User(1, "author", ["workspace"], False) checker = User(2, "checker", [], True) release_request1 = factories.create_release_request( @@ -263,30 +246,30 @@ def test_set_status(current, future, valid_author, valid_checker, api): ) if valid_author: - api.set_status(release_request1, future, user=author) + bll.set_status(release_request1, future, user=author) assert release_request1.status == future else: - with pytest.raises((api.InvalidStateTransition, api.RequestPermissionDenied)): - api.set_status(release_request1, future, user=author) + with pytest.raises((bll.InvalidStateTransition, bll.RequestPermissionDenied)): + bll.set_status(release_request1, future, user=author) if valid_checker: - api.set_status(release_request2, future, user=checker) + bll.set_status(release_request2, future, user=checker) assert release_request2.status == future else: - with pytest.raises((api.InvalidStateTransition, api.RequestPermissionDenied)): - api.set_status(release_request2, future, user=checker) + with pytest.raises((bll.InvalidStateTransition, bll.RequestPermissionDenied)): + bll.set_status(release_request2, future, user=checker) -def test_set_status_cannot_action_own_request(api): +def test_set_status_cannot_action_own_request(bll): user = User(2, "checker", [], True) release_request1 = factories.create_release_request( "workspace", user=user, status=Status.SUBMITTED ) - with pytest.raises(api.RequestPermissionDenied): - api.set_status(release_request1, Status.APPROVED, user=user) - with pytest.raises(api.RequestPermissionDenied): - api.set_status(release_request1, Status.REJECTED, user=user) + with pytest.raises(bll.RequestPermissionDenied): + bll.set_status(release_request1, Status.APPROVED, user=user) + with pytest.raises(bll.RequestPermissionDenied): + bll.set_status(release_request1, Status.REJECTED, user=user) release_request2 = factories.create_release_request( "workspace", @@ -294,11 +277,11 @@ def test_set_status_cannot_action_own_request(api): status=Status.APPROVED, ) - with pytest.raises(api.RequestPermissionDenied): - api.set_status(release_request2, Status.RELEASED, user=user) + with pytest.raises(bll.RequestPermissionDenied): + bll.set_status(release_request2, Status.RELEASED, user=user) -def test_add_file_to_request_not_author(api): +def test_add_file_to_request_not_author(bll): author = User(1, "author", ["workspace"], False) other = User(1, "other", ["workspace"], True) @@ -310,8 +293,8 @@ def test_add_file_to_request_not_author(api): user=author, ) - with pytest.raises(api.RequestPermissionDenied): - api.add_file_to_request(release_request, path, other) + with pytest.raises(bll.RequestPermissionDenied): + bll.add_file_to_request(release_request, path, other) @pytest.mark.parametrize( @@ -325,7 +308,7 @@ def test_add_file_to_request_not_author(api): (Status.WITHDRAWN, False), ], ) -def test_add_file_to_request_states(status, success, api): +def test_add_file_to_request_states(status, success, bll): author = User(1, "author", ["workspace"], False) path = Path("path/file.txt") @@ -338,11 +321,11 @@ def test_add_file_to_request_states(status, success, api): ) if success: - api.add_file_to_request(release_request, path, author) + bll.add_file_to_request(release_request, path, author) assert release_request.abspath("default" / path).exists() else: - with pytest.raises(api.RequestPermissionDenied): - api.add_file_to_request(release_request, path, author) + with pytest.raises(bll.RequestPermissionDenied): + bll.add_file_to_request(release_request, path, author) def test_request_release_invalid_state(): @@ -354,15 +337,15 @@ def test_request_release_invalid_state(): ) -def test_request_release_abspath(api): +def test_request_release_abspath(bll): path = UrlPath("foo/bar.txt") release_request = factories.create_release_request("id") factories.write_request_file(release_request, "default", path) - with pytest.raises(api.FileNotFound): + with pytest.raises(bll.FileNotFound): release_request.abspath("badgroup" / path) - with pytest.raises(api.FileNotFound): + with pytest.raises(bll.FileNotFound): release_request.abspath("default/does/not/exist") assert release_request.abspath("default" / path).exists() @@ -380,15 +363,15 @@ def setup_empty_release_request(): return release_request, path, author -def test_release_request_filegroups_with_no_files(api): +def test_release_request_filegroups_with_no_files(bll): release_request, _, _ = setup_empty_release_request() assert release_request.filegroups == {} -def test_release_request_filegroups_default_filegroup(api): +def test_release_request_filegroups_default_filegroup(bll): release_request, path, author = setup_empty_release_request() assert release_request.filegroups == {} - api.add_file_to_request(release_request, path, author) + bll.add_file_to_request(release_request, path, author) assert len(release_request.filegroups) == 1 filegroup = release_request.filegroups["default"] assert filegroup.name == "default" @@ -396,10 +379,10 @@ def test_release_request_filegroups_default_filegroup(api): assert filegroup.files[0].relpath == path -def test_release_request_filegroups_named_filegroup(api): +def test_release_request_filegroups_named_filegroup(bll): release_request, path, author = setup_empty_release_request() assert release_request.filegroups == {} - api.add_file_to_request(release_request, path, author, "test_group") + bll.add_file_to_request(release_request, path, author, "test_group") assert len(release_request.filegroups) == 1 filegroup = release_request.filegroups["test_group"] assert filegroup.name == "test_group" @@ -407,20 +390,20 @@ def test_release_request_filegroups_named_filegroup(api): assert filegroup.files[0].relpath == path -def test_release_request_filegroups_multiple_filegroups(api): +def test_release_request_filegroups_multiple_filegroups(bll): release_request, path, author = setup_empty_release_request() - api.add_file_to_request(release_request, path, author, "test_group") + bll.add_file_to_request(release_request, path, author, "test_group") assert len(release_request.filegroups) == 1 - workspace = api.get_workspace("workspace") + workspace = bll.get_workspace("workspace") path1 = Path("path/file1.txt") path2 = Path("path/file2.txt") factories.write_workspace_file(workspace, path1) factories.write_workspace_file(workspace, path2) - api.add_file_to_request(release_request, path1, author, "test_group") - api.add_file_to_request(release_request, path2, author, "test_group1") + bll.add_file_to_request(release_request, path1, author, "test_group") + bll.add_file_to_request(release_request, path2, author, "test_group1") - release_request = api.get_release_request(release_request.id) + release_request = bll.get_release_request(release_request.id) assert len(release_request.filegroups) == 2 release_request_files = { @@ -434,22 +417,22 @@ def test_release_request_filegroups_multiple_filegroups(api): } -def test_release_request_add_same_file(api): +def test_release_request_add_same_file(bll): release_request, path, author = setup_empty_release_request() assert release_request.filegroups == {} - api.add_file_to_request(release_request, path, author) + bll.add_file_to_request(release_request, path, author) assert len(release_request.filegroups) == 1 assert len(release_request.filegroups["default"].files) == 1 # Adding the same file again should not create a new RequestFile - with pytest.raises(api.APIException): - api.add_file_to_request(release_request, path, author) + with pytest.raises(bll.APIException): + bll.add_file_to_request(release_request, path, author) # We also can't add the same file to a different group - with pytest.raises(api.APIException): - api.add_file_to_request(release_request, path, author, "new_group") + with pytest.raises(bll.APIException): + bll.add_file_to_request(release_request, path, author, "new_group") - release_request = api.get_release_request(release_request.id) + release_request = bll.get_release_request(release_request.id) # No additional files or groups have been created assert len(release_request.filegroups) == 1 assert len(release_request.filegroups["default"].files) == 1