diff --git a/airlock/business_logic.py b/airlock/business_logic.py index 1969b8cf..daaf49ed 100644 --- a/airlock/business_logic.py +++ b/airlock/business_logic.py @@ -4,14 +4,11 @@ from dataclasses import dataclass, field from datetime import datetime from enum import Enum -from pathlib import Path - -# we use PurePosixPath as a convenient url path representation -from pathlib import PurePosixPath as UrlPath -from typing import Optional, Protocol +from pathlib import Path, PurePosixPath +from typing import TYPE_CHECKING, Protocol, Self, cast from django.conf import settings -from django.shortcuts import reverse +from django.urls import reverse from django.utils.functional import SimpleLazyObject from django.utils.module_loading import import_string @@ -20,6 +17,17 @@ from airlock.users import User +# We use PurePosixPath as a convenient URL path representation. In theory we could use +# `NewType` here to indicate that we want this to be treated as a distinct type without +# actually creating one. But doing so results in a number of spurious type errors for +# reasons I don't fully understand (possibly because PurePosixPath isn't itself type +# annotated?). +if TYPE_CHECKING: # pragma: no cover + + class UrlPath(PurePosixPath): ... +else: + UrlPath = PurePosixPath + ROOT_PATH = UrlPath() # empty path @@ -75,6 +83,9 @@ def get_contents_url( def is_supporting_file(self, relpath: UrlPath): """Is this path a supporting file?""" + def abspath(self, path: UrlPath) -> Path: + """Get the absolute path of the container object with path""" + @dataclass(order=True) class Workspace: @@ -85,7 +96,7 @@ class Workspace: """ name: str - metadata: dict = field(default_factory=dict) + metadata: dict[str, str] = field(default_factory=dict) # can be set to mark the currently selected path in this workspace selected_path: UrlPath = ROOT_PATH @@ -172,7 +183,7 @@ class RequestFile: filetype: RequestFileType = RequestFileType.OUTPUT @classmethod - def from_dict(cls, attrs): + def from_dict(cls, attrs) -> Self: return cls( **{k: v for k, v in attrs.items() if k != "reviews"}, reviews=[FileReview.from_dict(value) for value in attrs.get("reviews", ())], @@ -186,7 +197,7 @@ class FileGroup: """ name: str - files: dict[RequestFile] + files: dict[UrlPath, RequestFile] @property def output_files(self): @@ -199,7 +210,7 @@ def supporting_files(self): ] @classmethod - def from_dict(cls, attrs): + def from_dict(cls, attrs) -> Self: return cls( **{k: v for k, v in attrs.items() if k != "files"}, files={ @@ -224,13 +235,13 @@ class ReleaseRequest: author: str created_at: datetime status: RequestStatus = RequestStatus.PENDING - filegroups: dict[FileGroup] = field(default_factory=dict) + filegroups: dict[str, FileGroup] = field(default_factory=dict) # can be set to mark the currently selected path in this release request selected_path: UrlPath = ROOT_PATH @classmethod - def from_dict(cls, attrs): + def from_dict(cls, attrs) -> Self: return cls( **{k: v for k, v in attrs.items() if k != "filegroups"}, filegroups=cls._filegroups_from_dict(attrs.get("filegroups", {})), @@ -346,12 +357,48 @@ def store_file(release_request: ReleaseRequest, abspath: Path) -> str: return digest -class DataAccessLayerProtocol: +class DataAccessLayerProtocol(Protocol): """ - 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. + Structural type class for the Data Access Layer + + Implementations aren't obliged to subclass this as long as they implement the + specified methods, though it may be clearer to do so. """ + def get_release_request(self, request_id: str): + raise NotImplementedError() + + def create_release_request(self, **kwargs): + raise NotImplementedError() + + def get_active_requests_for_workspace_by_user(self, workspace: str, username: str): + raise NotImplementedError() + + def get_requests_authored_by_user(self, username: str): + raise NotImplementedError() + + def get_outstanding_requests_for_review(self): + raise NotImplementedError() + + def set_status(self, request_id: str, status: RequestStatus): + raise NotImplementedError() + + def add_file_to_request( + self, + request_id, + relpath: UrlPath, + file_id: str, + group_name: str, + filetype: RequestFileType, + ): + raise NotImplementedError() + + def approve_file(self, request_id: str, relpath: UrlPath, username: str): + raise NotImplementedError() + + def reject_file(self, request_id: str, relpath: UrlPath, username: str): + raise NotImplementedError() + class BusinessLogicLayer: """ @@ -437,38 +484,47 @@ def get_release_request(self, request_id: str, user: User) -> ReleaseRequest: return release_request def get_current_request( - self, workspace_name: str, user: User, create: bool = False - ) -> ReleaseRequest: - """Get the current request for the a workspace/user. - - If create is True, create one. - """ + self, workspace_name: str, user: User + ) -> ReleaseRequest | None: + """Get the current request for a workspace/user.""" 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}" - ) + if n == 0: + return None 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 + raise Exception( + f"Multiple active release requests for user {user.username} in " + f"workspace {workspace_name}" + ) + + def get_or_create_current_request( + self, workspace_name: str, user: User + ) -> ReleaseRequest: + """ + Get the current request for a workspace/user, or create a new one if there is + none. + """ + request = self.get_current_request(workspace_name, user) + if request is not None: + return request + + # 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) def get_requests_authored_by_user(self, user: User) -> list[ReleaseRequest]: """Get all current requests authored by user.""" @@ -582,7 +638,7 @@ def add_file_to_request( release_request: ReleaseRequest, relpath: UrlPath, user: User, - group_name: Optional[str] = "default", + group_name: str = "default", filetype: RequestFileType = RequestFileType.OUTPUT, ): if user.username != release_request.author: @@ -663,7 +719,7 @@ def approve_file( self._verify_permission_to_review_file(release_request, relpath, user) - bll._dal.approve_file(release_request.id, relpath, user) + bll._dal.approve_file(release_request.id, relpath, user.username) def reject_file( self, release_request: ReleaseRequest, relpath: UrlPath, user: User @@ -672,7 +728,7 @@ def reject_file( self._verify_permission_to_review_file(release_request, relpath, user) - bll._dal.reject_file(release_request.id, relpath, user) + bll._dal.reject_file(release_request.id, relpath, user.username) def _get_configured_bll(): @@ -681,5 +737,7 @@ def _get_configured_bll(): # 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) +# access so as to avoid reading `settings` during import. The `cast` here is a runtime +# no-op, but indicates to the type-checker that this should be treated as an instance of +# BusinessLogicLayer not SimpleLazyObject. +bll = cast(BusinessLogicLayer, SimpleLazyObject(_get_configured_bll)) diff --git a/airlock/file_browser_api.py b/airlock/file_browser_api.py index 8aa61eb6..eba7decb 100644 --- a/airlock/file_browser_api.py +++ b/airlock/file_browser_api.py @@ -1,3 +1,5 @@ +from __future__ import annotations + from dataclasses import dataclass, field from enum import Enum @@ -35,9 +37,9 @@ class PathNotFound(Exception): container: AirlockContainer relpath: UrlPath - type: PathType = None - children: list["PathItem"] = field(default_factory=list) - parent: "PathItem" = None + type: PathType | None = None + children: list[PathItem] = field(default_factory=list) + parent: PathItem | None = None # is this the currently selected path? selected: bool = False @@ -49,7 +51,7 @@ class PathNotFound(Exception): # what to display for this node when rendering the tree. Defaults to name, # but this allow it to be overridden. - display_text: str = None + display_text: str | None = None def __post_init__(self): # ensure is UrlPath @@ -76,18 +78,18 @@ def display(self): def url(self): suffix = "/" if self.is_directory() else "" - return self.container.get_url(f"{self.relpath}{suffix}") + return self.container.get_url(self.relpath) + suffix def contents_url(self, download=False): if self.type != PathType.FILE: raise Exception(f"contents_url called on non-file path {self.relpath}") - return self.container.get_contents_url(f"{self.relpath}", download=download) + return self.container.get_contents_url(self.relpath, download=download) def download_url(self): return self.contents_url(download=True) def siblings(self): - if not self.relpath.parents: + if self.parent is None: return [] else: return self.parent.children @@ -132,7 +134,7 @@ def html_classes(self): distinguish file/dirs, and maybe even file types, in the UI, in case we need to. """ - classes = [self.type.value.lower()] + classes = [self.type.value.lower()] if self.type else [] if self.type == PathType.FILE: classes.append(self.file_type()) @@ -330,14 +332,16 @@ def get_path_tree( ): """Walk a flat list of paths and create a tree from them.""" - def build_path_tree(path_parts, parent): + def build_path_tree( + path_parts: list[list[str]], parent: PathItem + ) -> list[PathItem]: # group multiple paths into groups by first part of path - grouped = dict() - for child, *descendants in path_parts: + grouped: dict[str, list[list[str]]] = dict() + for child, *descendant_parts in path_parts: if child not in grouped: grouped[child] = [] - if descendants: - grouped[child].append(descendants) + if descendant_parts: + grouped[child].append(descendant_parts) tree = [] diff --git a/airlock/forms.py b/airlock/forms.py index 4ec6ba51..76ffa329 100644 --- a/airlock/forms.py +++ b/airlock/forms.py @@ -24,15 +24,15 @@ def __init__(self, *args, **kwargs): self.filegroup_names = release_request.filegroups.keys() else: self.filegroup_names = set() - group_choices = { - (name, name) for name in self.filegroup_names if name != "default" - } - group_choices = [("default", "default"), *sorted(group_choices)] + group_names = sorted(self.filegroup_names - {"default"}) + group_choices = [(name, name) for name in ["default", *group_names]] + # Use type narrowing to persuade mpy this has a `choices` attr + assert isinstance(self.fields["filegroup"], forms.ChoiceField) self.fields["filegroup"].choices = group_choices self.fields["new_filegroup"] def clean_new_filegroup(self): - new_filegroup = self.cleaned_data.get("new_filegroup").lower() + new_filegroup = self.cleaned_data.get("new_filegroup", "").lower() if new_filegroup in [fg.lower() for fg in self.filegroup_names]: self.add_error( "new_filegroup", diff --git a/airlock/login_api.py b/airlock/login_api.py index 7069db2b..4aca5463 100644 --- a/airlock/login_api.py +++ b/airlock/login_api.py @@ -1,4 +1,5 @@ import json +from pathlib import Path import requests from django.conf import settings @@ -13,7 +14,7 @@ class LoginError(Exception): def get_user_data(user: str, token: str): if settings.AIRLOCK_DEV_USERS_FILE and not settings.AIRLOCK_API_TOKEN: - return get_user_data_dev(user, token) + return get_user_data_dev(settings.AIRLOCK_DEV_USERS_FILE, user, token) else: return get_user_data_prod(user, token) @@ -31,9 +32,9 @@ def get_user_data_prod(user: str, token: str): return response.json() -def get_user_data_dev(user: str, token: str): +def get_user_data_dev(dev_users_file: Path, user: str, token: str): try: - dev_users = json.loads(settings.AIRLOCK_DEV_USERS_FILE.read_text()) + dev_users = json.loads(dev_users_file.read_text()) except FileNotFoundError as e: # pragma: no cover e.add_note( "You may want to run:\n\n just load-example-data\n\nto create one." diff --git a/airlock/middleware.py b/airlock/middleware.py index 590c6794..18bae4ec 100644 --- a/airlock/middleware.py +++ b/airlock/middleware.py @@ -1,6 +1,7 @@ from urllib.parse import urlencode -from django.shortcuts import redirect, reverse +from django.shortcuts import redirect +from django.urls import reverse from airlock.users import User diff --git a/airlock/nav.py b/airlock/nav.py index 3adb6fdb..04e273c5 100644 --- a/airlock/nav.py +++ b/airlock/nav.py @@ -1,6 +1,7 @@ from collections.abc import Callable from dataclasses import dataclass +from django.http import HttpRequest from django.urls import reverse @@ -12,7 +13,7 @@ def default_predicate(request): class NavItem: name: str url_name: str - predicate: Callable = default_predicate + predicate: Callable[[HttpRequest], bool] = default_predicate def iter_nav(items, request): diff --git a/airlock/renderers.py b/airlock/renderers.py index 9e796689..96a7ad21 100644 --- a/airlock/renderers.py +++ b/airlock/renderers.py @@ -5,8 +5,9 @@ from email.utils import formatdate from functools import cached_property from pathlib import Path +from typing import ClassVar, Self, cast -from django.http import FileResponse +from django.http import FileResponse, HttpResponseBase from django.template import Template, loader from django.template.response import SimpleTemplateResponse @@ -14,12 +15,13 @@ @dataclass class RendererTemplate: name: str - path: Path = None - template: Template = None + path: Path + template: Template - def __post_init__(self): - self.template = loader.get_template(self.name) - self.path = Path(self.template.template.origin.name) + @classmethod + def from_name(cls, name: str) -> Self: + template = cast(Template, loader.get_template(name)) + return cls(name, template=template, path=Path(template.origin.name)) def cache_id(self): return filesystem_key(self.path.stat()) @@ -28,7 +30,7 @@ def cache_id(self): @dataclass class Renderer: MAX_AGE = 365 * 24 * 60 * 60 # 1 year - template = None + template: ClassVar[RendererTemplate | None] = None abspath: Path file_cache_id: str @@ -38,7 +40,9 @@ class Renderer: def get_response(self): if self.template: context = self.context() - response = SimpleTemplateResponse(self.template.template, context) + response: HttpResponseBase = SimpleTemplateResponse( + self.template.template, context + ) else: response = FileResponse(self.abspath.open("rb"), filename=self.filename) @@ -72,7 +76,7 @@ def headers(self): class CSVRenderer(Renderer): - template = RendererTemplate("file_browser/csv.html") + template = RendererTemplate.from_name("file_browser/csv.html") def context(self): reader = csv.reader(self.abspath.open()) @@ -81,7 +85,7 @@ def context(self): class TextRenderer(Renderer): - template = RendererTemplate("file_browser/text.html") + template = RendererTemplate.from_name("file_browser/text.html") def context(self): return { diff --git a/airlock/users.py b/airlock/users.py index 12cded9a..b65f5686 100644 --- a/airlock/users.py +++ b/airlock/users.py @@ -10,7 +10,7 @@ class User: """ username: str - workspaces: dict = dataclasses.field(default_factory=dict) + workspaces: dict[str, dict[str, str]] = dataclasses.field(default_factory=dict) output_checker: bool = dataclasses.field(default=False) def __post_init__(self): diff --git a/airlock/views/__init__.py b/airlock/views/__init__.py index 5017c4e7..e0910216 100644 --- a/airlock/views/__init__.py +++ b/airlock/views/__init__.py @@ -14,3 +14,20 @@ workspace_index, workspace_view, ) + + +__all__ = [ + "login", + "logout", + "index", + "request_contents", + "request_index", + "request_reject", + "request_release_files", + "request_submit", + "request_view", + "workspace_add_file_to_request", + "workspace_contents", + "workspace_index", + "workspace_view", +] diff --git a/airlock/views/workspace.py b/airlock/views/workspace.py index e972c85b..409b5c93 100644 --- a/airlock/views/workspace.py +++ b/airlock/views/workspace.py @@ -125,11 +125,13 @@ def workspace_add_file_to_request(request, workspace_name): except bll.FileNotFound: raise Http404() - release_request = bll.get_current_request(workspace_name, request.user, create=True) + release_request = bll.get_or_create_current_request(workspace_name, request.user) form = AddFileForm(request.POST, release_request=release_request) if form.is_valid(): - group_name = form.cleaned_data.get("new_filegroup") or form.cleaned_data.get( - "filegroup" + group_name = ( + form.cleaned_data.get("new_filegroup") + or form.cleaned_data.get("filegroup") + or "" ) filetype = RequestFileType[form.cleaned_data["filetype"]] try: @@ -146,8 +148,9 @@ def workspace_add_file_to_request(request, workspace_name): f"{filetype.name.title()} file has been added to request (file group '{group_name}')", ) else: - for error in form.errors.values(): - messages.error(request, error) + for error_list in form.errors.values(): + for error in error_list: + messages.error(request, str(error)) # Redirect to the file in the workspace return redirect(workspace.get_url(relpath)) diff --git a/justfile b/justfile index 96a95081..d34b279a 100644 --- a/justfile +++ b/justfile @@ -105,6 +105,7 @@ check: devenv check "$BIN/djhtml --tabwidth 2 --check airlock/" check "docker run --rm -i ghcr.io/hadolint/hadolint:v2.12.0-alpine < docker/Dockerfile" check "find docker/ airlock/ job-server -name \*.sh -print0 | xargs -0 docker run --rm -v \"$PWD:/mnt\" koalaman/shellcheck:v0.9.0" + check "$BIN/mypy airlock/ local_db/" if [[ $failed > 0 ]]; then echo -en "\e[1;31m" @@ -114,6 +115,11 @@ check: devenv fi +# run mypy type checker +mypy *ARGS: devenv + $BIN/mypy airlock/ local_db/ "$@" + + # fix formatting and import sort ordering fix: devenv $BIN/ruff format . diff --git a/local_db/data_access.py b/local_db/data_access.py index 838441d5..0730bac2 100644 --- a/local_db/data_access.py +++ b/local_db/data_access.py @@ -5,13 +5,14 @@ from airlock.business_logic import ( BusinessLogicLayer, DataAccessLayerProtocol, + FileReviewStatus, RequestFileType, RequestStatus, + UrlPath, ) from local_db.models import ( FileGroupMetadata, FileReview, - FileReviewStatus, RequestFileMetadata, RequestMetadata, ) @@ -22,7 +23,7 @@ class LocalDBDataAccessLayer(DataAccessLayerProtocol): Implementation of DataAccessLayerProtocol using local_db models to store data """ - def _request(self, metadata: RequestMetadata = None): + def _request(self, metadata: RequestMetadata): """Unpack the db data into the Request object.""" return dict( id=metadata.id, @@ -125,10 +126,10 @@ def set_status(self, request_id: str, status: RequestStatus): def add_file_to_request( self, request_id, - relpath: Path, + relpath: UrlPath, file_id: str, group_name: str, - filetype=RequestFileType, + filetype: RequestFileType, ): with transaction.atomic(): # Get/create the FileGroupMetadata if it doesn't already exist @@ -162,7 +163,7 @@ def add_file_to_request( metadata = self._find_metadata(request_id) return self._get_filegroups(metadata) - def approve_file(self, request_id, relpath, user): + def approve_file(self, request_id: str, relpath: UrlPath, username: str): with transaction.atomic(): # nb. the business logic layer approve_file() should confirm that this path # is part of the request before calling this method @@ -171,19 +172,19 @@ def approve_file(self, request_id, relpath, user): ) review, _ = FileReview.objects.get_or_create( - file=request_file, reviewer=user.username + file=request_file, reviewer=username ) review.status = FileReviewStatus.APPROVED review.save() - def reject_file(self, request_id, relpath, user): + def reject_file(self, request_id: str, relpath: UrlPath, username: str): with transaction.atomic(): request_file = RequestFileMetadata.objects.get( filegroup__request_id=request_id, relpath=relpath ) review, _ = FileReview.objects.get_or_create( - file=request_file, reviewer=user.username + file=request_file, reviewer=username ) review.status = FileReviewStatus.REJECTED review.save() diff --git a/local_db/models.py b/local_db/models.py index bcba3c08..6d5e836a 100644 --- a/local_db/models.py +++ b/local_db/models.py @@ -1,6 +1,12 @@ +import enum +from typing import TYPE_CHECKING + from django.db import models from django.utils import timezone -from ulid import ulid + +# We could create our own type stubs for this module but our use of it is so simple that +# it's not really worth it +from ulid import ulid # type: ignore from airlock.business_logic import FileReviewStatus, RequestFileType, RequestStatus @@ -9,7 +15,32 @@ def local_request_id(): return str(ulid()) -class EnumField(models.TextField): +if TYPE_CHECKING: # pragma: no cover + # This works around a couple of issues with Django/Django-stubs/mypy: + # + # 1. Django-stubs requires `Field` subclasses to provide type arguments, but the + # actual Django `Field` class doesn't accept type arguments so it will pass type + # checking but fail at runtime. You can work around this by applying + # `django_stubs_ext.monkeypatch()` but yuck no thanks. + # + # 2. Django-stubs sets the type arguments on `TextField` to `str` and we don't seem + # to be able to override this to say that we accept/return enums. + # + # Even so, the type signature below is not actually quite what we want. Each + # instance of `EnumField` accepts/returns just a single class of enum, not all enums + # in general. And it should be a type error to use the wrong kind of enum with the + # field. It's perfectly possible to specify this kind of behaviour in Python using + # generics and type variables, but for whatever reason Django-stubs doesn't support + # this (see issues below) so we just enforce that the field is used with _some_ enum + # class rather than, say, a string – which should at least catch some errors. + # https://github.com/typeddjango/django-stubs/issues/545 + # https://github.com/typeddjango/django-stubs/issues/336 + BaseTextField = models.Field[enum.Enum, enum.Enum] +else: + BaseTextField = models.TextField + + +class EnumField(BaseTextField): """Custom field that ensures correct types for a column, defined by an Enum. Specifically, data is stored in the db as the string name, e.g. diff --git a/old_api/__init__.py b/old_api/__init__.py index a8d7b577..b4821741 100644 --- a/old_api/__init__.py +++ b/old_api/__init__.py @@ -1,10 +1,11 @@ import hashlib from datetime import datetime, timezone +from pathlib import Path import requests from django.conf import settings -from old_api.schema import FileList, FileMetadata +from old_api.schema import FileList, FileMetadata, UrlFileName session = requests.Session() @@ -16,11 +17,15 @@ def create_filelist(paths): for relpath, abspath in paths: files.append( FileMetadata( - name=str(relpath), + name=UrlFileName(relpath), size=abspath.stat().st_size, sha256=hashlib.sha256(abspath.read_bytes()).hexdigest(), - date=modified_time(abspath), - url=str(relpath), # not needed, but has to be set + # The schema is defined to take a datetime here but we're giving it a + # string. Given that this is legacy code which interacts with an + # external API and manifestly _does_ work, we'd rather leave it as is + # that make changes which risk changing the output format. + date=modified_time(abspath), # type: ignore[arg-type] + url=UrlFileName(relpath), # not needed, but has to be set metadata={"tool": "airlock"}, ) ) @@ -63,6 +68,6 @@ def upload_file(release_id, relpath, abspath, username): return response -def modified_time(path): +def modified_time(path: Path) -> str: mtime = path.stat().st_mtime return datetime.fromtimestamp(mtime, tz=timezone.utc).isoformat() diff --git a/old_api/schema.py b/old_api/schema.py index d591cc97..c9716219 100644 --- a/old_api/schema.py +++ b/old_api/schema.py @@ -31,19 +31,19 @@ class ReviewStatus(Enum): class FileReview(BaseModel): status: ReviewStatus - comments: dict + comments: dict[str, str] class FileMetadata(BaseModel): """Metadata for a workspace file.""" name: UrlFileName - url: UrlFileName = None # Url to path on release-hatch instance + url: UrlFileName | None = None # Url to path on release-hatch instance size: int # size in bytes sha256: str # sha256 of file date: datetime # last modified in ISO date format - metadata: dict = None # user supplied metadata about this file - review: FileReview = None # any review metadata for this file + metadata: dict[str, str] | None = None # user supplied metadata about this file + review: FileReview | None = None # any review metadata for this file class FileList(BaseModel): @@ -53,8 +53,8 @@ class FileList(BaseModel): """ files: list[FileMetadata] - metadata: dict = None # user supplied metadata about thse Release - review: dict = None # review comments for the whole Release + metadata: dict[str, str] | None = None # user supplied metadata about thse Release + review: dict[str, str] | None = None # review comments for the whole Release def get(self, name): # pragma: no cover name = str(name) diff --git a/pyproject.toml b/pyproject.toml index 04ccc2a2..ec830efd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -57,3 +57,21 @@ exclude_blocks = [ [tool.ruff.lint.per-file-ignores] "airlock/views/__init__.py" = ["F401"] + +[tool.mypy] +plugins = ["mypy_django_plugin.main"] +disallow_any_generics = true +no_implicit_reexport = true +warn_return_any = true +check_untyped_defs = true + +# Don't follow the import chain into the modules containing code vendored from elsewhere +[[tool.mypy.overrides]] +module = "assets.base_views" +follow_imports = "skip" +[[tool.mypy.overrides]] +module = "services.tracing" +follow_imports = "skip" + +[tool.django-stubs] +django_settings_module = "airlock.settings" diff --git a/requirements.dev.in b/requirements.dev.in index b84a0fe2..bfe56f97 100644 --- a/requirements.dev.in +++ b/requirements.dev.in @@ -15,3 +15,8 @@ responses # Currently using our fork of django_coverage_plugin, pending # upstream PR https://github.com/nedbat/django_coverage_plugin/pull/93 https://github.com/opensafely-core/django_coverage_plugin/archive/153a0ca6c02f7f01831568a546c848c4a3f082cd.zip + +# Type-checking and type stubs +mypy +django-stubs[compatible-mypy] +types-requests diff --git a/requirements.dev.txt b/requirements.dev.txt index a9e90987..8fa651ec 100644 --- a/requirements.dev.txt +++ b/requirements.dev.txt @@ -180,6 +180,8 @@ django==5.0.3 \ # via # -c requirements.prod.txt # django-debug-toolbar + # django-stubs + # django-stubs-ext django-coverage-plugin @ https://github.com/opensafely-core/django_coverage_plugin/archive/153a0ca6c02f7f01831568a546c848c4a3f082cd.zip \ --hash=sha256:db70db8737dd269c346f7af6e0c735a3d6462b944302f6bc0178c7bac9a4c7ee # via -r requirements.dev.in @@ -193,6 +195,14 @@ django-debug-toolbar-template-profiler==2.1.0 \ --hash=sha256:740d4fa90f9c72fe97c896d5a395abc535cf8e9d571c1710186aa775a861d7f4 \ --hash=sha256:7c77b3d96a0f64a47ed80dec88bad0832fe2834d17da5cd77f86639680b56a8b # via -r requirements.dev.in +django-stubs==4.2.7 \ + --hash=sha256:4cf4de258fa71adc6f2799e983091b9d46cfc67c6eebc68fe111218c9a62b3b8 \ + --hash=sha256:8ccd2ff4ee5adf22b9e3b7b1a516d2e1c2191e9d94e672c35cc2bc3dd61e0f6b + # via -r requirements.dev.in +django-stubs-ext==4.2.7 \ + --hash=sha256:45a5d102417a412e3606e3c358adb4744988a92b7b58ccf3fd64bddd5d04d14c \ + --hash=sha256:519342ac0849cda1559746c9a563f03ff99f636b0ebe7c14b75e816a00dfddc3 + # via django-stubs djhtml==3.0.6 \ --hash=sha256:abfc4d7b4730432ca6a98322fbdf8ae9d6ba254ea57ba3759a10ecb293bc57de # via -r requirements.dev.in @@ -266,6 +276,41 @@ iniconfig==2.0.0 \ --hash=sha256:2d91e135bf72d31a410b17c16da610a82cb55f6b0477d1a902134b24a455b8b3 \ --hash=sha256:b6a85871a79d2e3b22d2d1b94ac2824226a63c6b741c88f7ae975f18b6778374 # via pytest +mypy==1.7.1 \ + --hash=sha256:12cce78e329838d70a204293e7b29af9faa3ab14899aec397798a4b41be7f340 \ + --hash=sha256:1484b8fa2c10adf4474f016e09d7a159602f3239075c7bf9f1627f5acf40ad49 \ + --hash=sha256:204e0d6de5fd2317394a4eff62065614c4892d5a4d1a7ee55b765d7a3d9e3f82 \ + --hash=sha256:2643d145af5292ee956aa0a83c2ce1038a3bdb26e033dadeb2f7066fb0c9abce \ + --hash=sha256:2c6e4464ed5f01dc44dc9821caf67b60a4e5c3b04278286a85c067010653a0eb \ + --hash=sha256:2f7f6985d05a4e3ce8255396df363046c28bea790e40617654e91ed580ca7c51 \ + --hash=sha256:31902408f4bf54108bbfb2e35369877c01c95adc6192958684473658c322c8a5 \ + --hash=sha256:40716d1f821b89838589e5b3106ebbc23636ffdef5abc31f7cd0266db936067e \ + --hash=sha256:4b901927f16224d0d143b925ce9a4e6b3a758010673eeded9b748f250cf4e8f7 \ + --hash=sha256:4fc3d14ee80cd22367caaaf6e014494415bf440980a3045bf5045b525680ac33 \ + --hash=sha256:5cf3f0c5ac72139797953bd50bc6c95ac13075e62dbfcc923571180bebb662e9 \ + --hash=sha256:6dbdec441c60699288adf051f51a5d512b0d818526d1dcfff5a41f8cd8b4aaf1 \ + --hash=sha256:72cf32ce7dd3562373f78bd751f73c96cfb441de147cc2448a92c1a308bd0ca6 \ + --hash=sha256:75aa828610b67462ffe3057d4d8a4112105ed211596b750b53cbfe182f44777a \ + --hash=sha256:75c4d2a6effd015786c87774e04331b6da863fc3fc4e8adfc3b40aa55ab516fe \ + --hash=sha256:78e25b2fd6cbb55ddfb8058417df193f0129cad5f4ee75d1502248e588d9e0d7 \ + --hash=sha256:84860e06ba363d9c0eeabd45ac0fde4b903ad7aa4f93cd8b648385a888e23200 \ + --hash=sha256:8c5091ebd294f7628eb25ea554852a52058ac81472c921150e3a61cdd68f75a7 \ + --hash=sha256:944bdc21ebd620eafefc090cdf83158393ec2b1391578359776c00de00e8907a \ + --hash=sha256:9c7ac372232c928fff0645d85f273a726970c014749b924ce5710d7d89763a28 \ + --hash=sha256:d9b338c19fa2412f76e17525c1b4f2c687a55b156320acb588df79f2e6fa9fea \ + --hash=sha256:ee5d62d28b854eb61889cde4e1dbc10fbaa5560cb39780c3995f6737f7e82120 \ + --hash=sha256:f2c2521a8e4d6d769e3234350ba7b65ff5d527137cdcde13ff4d99114b0c8e7d \ + --hash=sha256:f6efc9bd72258f89a3816e3a98c09d36f079c223aa345c659622f056b760ab42 \ + --hash=sha256:f7c5d642db47376a0cc130f0de6d055056e010debdaf0707cd2b0fc7e7ef30ea \ + --hash=sha256:fcb6d9afb1b6208b4c712af0dafdc650f518836065df0d4fb1d800f5d6773db2 \ + --hash=sha256:fcd2572dd4519e8a6642b733cd3a8cfc1ef94bafd0c1ceed9c94fe736cb65b6a + # via + # -r requirements.dev.in + # django-stubs +mypy-extensions==1.0.0 \ + --hash=sha256:4392f6c0eb8a5668a69e23d168ffa70f0be9ccfd32b5cc2d26a34ae5b844552d \ + --hash=sha256:75dbf8955dc00442a438fc4d0666508a9a97b6bd41aa2f0ffe9d2f2725af0782 + # via mypy packaging==24.0 \ --hash=sha256:2ddfb553fdf02fb784c234c7ba6ccc288296ceabec964ad2eae3777778130bc5 \ --hash=sha256:eb82c5e3e56209074766e6885bb04b8c38a0c015d0a30036ebe7ece34c9989e9 @@ -425,11 +470,26 @@ text-unidecode==1.3 \ --hash=sha256:1311f10e8b895935241623731c2ba64f4c455287888b18189350b67134a822e8 \ --hash=sha256:bad6603bb14d279193107714b288be206cac565dfa49aa5b105294dd5c4aab93 # via python-slugify +types-pytz==2024.1.0.20240203 \ + --hash=sha256:9679eef0365db3af91ef7722c199dbb75ee5c1b67e3c4dd7bfbeb1b8a71c21a3 \ + --hash=sha256:c93751ee20dfc6e054a0148f8f5227b9a00b79c90a4d3c9f464711a73179c89e + # via django-stubs +types-pyyaml==6.0.12.20240311 \ + --hash=sha256:a9e0f0f88dc835739b0c1ca51ee90d04ca2a897a71af79de9aec5f38cb0a5342 \ + --hash=sha256:b845b06a1c7e54b8e5b4c683043de0d9caf205e7434b3edc678ff2411979b8f6 + # via django-stubs +types-requests==2.31.0.20240311 \ + --hash=sha256:47872893d65a38e282ee9f277a4ee50d1b28bd592040df7d1fdaffdf3779937d \ + --hash=sha256:b1c1b66abfb7fa79aae09097a811c4aa97130eb8831c60e47aee4ca344731ca5 + # via -r requirements.dev.in typing-extensions==4.10.0 \ --hash=sha256:69b1a937c3a517342112fb4c6df7e72fc39a38e7891a5730ed4985b5214b5475 \ --hash=sha256:b0abd7c89e8fb96f98db18d86106ff1d90ab692004eb746cf6eda2682f91b3cb # via # -c requirements.prod.txt + # django-stubs + # django-stubs-ext + # mypy # pyee urllib3==2.2.1 \ --hash=sha256:450b20ec296a467077128bff42b73080516e71b56ff59a60a02bef2232c4fa9d \ @@ -438,6 +498,7 @@ urllib3==2.2.1 \ # -c requirements.prod.txt # requests # responses + # types-requests wheel==0.43.0 \ --hash=sha256:465ef92c69fa5c5da2d1cf8ac40559a8c940886afcef87dcf14b9470862f1d85 \ --hash=sha256:55c570405f142630c6b9f72fe09d9b67cf1477fcf543ae5b8dcb1f5b7377da81 @@ -513,7 +574,9 @@ wrapt==1.16.0 \ --hash=sha256:f6b2d0c6703c988d334f297aa5df18c45e97b0af3679bb75059e0e0bd8b1069d \ --hash=sha256:f8212564d49c50eb4565e502814f694e240c55551a5f1bc841d4fcaabb0a9b8a \ --hash=sha256:ffa565331890b90056c01db69c0fe634a776f8019c143a5ae265f9c6bc4bd6d4 - # via django-debug-toolbar-template-profiler + # via + # -c requirements.prod.txt + # django-debug-toolbar-template-profiler # The following packages are considered to be unsafe in a requirements file: pip==24.0 \ @@ -523,4 +586,6 @@ pip==24.0 \ setuptools==69.2.0 \ --hash=sha256:0ff4183f8f42cd8fa3acea16c45205521a4ef28f73c6391d8a25e92893134f2e \ --hash=sha256:c21c49fb1042386df081cb5d86759792ab89efca84cf114889191cd09aacc80c - # via pip-tools + # via + # -c requirements.prod.txt + # pip-tools diff --git a/tests/unit/test_business_logic.py b/tests/unit/test_business_logic.py index bcafcf4d..51fb5218 100644 --- a/tests/unit/test_business_logic.py +++ b/tests/unit/test_business_logic.py @@ -213,7 +213,7 @@ def test_provider_get_current_request_for_user(bll): factories.create_release_request(workspace, other_user) assert bll.get_current_request("workspace", user) is None - release_request = bll.get_current_request("workspace", user, create=True) + release_request = bll.get_or_create_current_request("workspace", user) assert release_request.workspace == "workspace" assert release_request.author == user.username @@ -230,7 +230,7 @@ def test_provider_get_current_request_for_user_output_checker(bll): user = factories.create_user("output_checker", [], True) with pytest.raises(bll.RequestPermissionDenied): - bll.get_current_request("workspace", user, create=True) + bll.get_or_create_current_request("workspace", user) @pytest.mark.parametrize(