Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/docs/adr/000-vertical-slice-architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,14 @@ damnit_api/
├── core/ # framework-free, imports nothing app-specific:
│ ├── errors.py # Shared error classes; see ADR-001
│ ├── types.py # `ProposalNumber`, pure value types (no I/O)
│ ├── types.py # `ProposalNumber`, pure value types (no I/O); see ADR-004
│ ├── const.py # DamnitType etc.
│ └── conversions.py # b64image, blob2numpy, type mapping
├── proposals/ # proposal identity, metadata, discovery
│ ├── models.py # `ProposalMeta` + domain models
│ ├── services.py # fetch/cache/upsert proposal metadata (auth-free)
│ ├── locator.py # `ProposalPathLocator` implementations
│ ├── locator.py # `ProposalPathLocator` implementations; see ADR-004
│ ├── routers.py
│ └── gql.py
Expand Down
35 changes: 35 additions & 0 deletions api/docs/adr/004-proposal-path-locator.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
---
date: 2026-07-07
---

# ADR-004 - Proposal path resolution: pure value type, injectable locator

## Context and Problem Statement

A proposal number arrives as an untyped string or integer and is used in three ways: as a GraphQL input, as a filesystem path to the proposal's DAMNIT directory, and as a persistence key. Turning a number into a directory requires filesystem probing on GPFS - globbing under the proposal root, checking candidate directories, stat-ing for read-only status. GPFS is a network filesystem, so those calls can stall for seconds, and a hung mount can stall them indefinitely.

Two questions follow. Where does a proposal number get validated and formatted? Where does number-to-directory resolution live?

## Considered Options

- One `ProposalNumber` type that both validates and resolves its own path, reading configuration directly (the reference tree's approach: a blocking `find_damnit_path_sync` method).
- A pure `ProposalNumber` value type plus a separate injectable `ProposalPathLocator` that owns resolution.

## Decision Outcome

Chosen option: a pure value type plus a separate locator, because it keeps the value type free of I/O and settings, and isolates slow GPFS access behind a swappable, testable seam.

### Consequences

- Good: `ProposalNumber` validation and formatting are pure and trivially unit-testable, with no settings coupling.
- Good: path heuristics live in one place behind a protocol, swappable for local mode and for tests.
- Bad: resolution becomes asynchronous once fully adopted, which will ripple into repository acquisition and the resolvers that call it.

## Details

### The rules

1. `ProposalNumber` (target `core/types.py`, today `shared/models.py`) is a pure value type. It validates (1-999999, rejects floats), formats to the canonical `p{n:06d}` form, and carries a pydantic schema. It does no I/O and reads no settings.
2. Number-to-directory resolution belongs in a `ProposalPathLocator` (target `proposals/locator.py`), built by a factory, held on `AppState`, and injected where resolution is needed. Implementations are a GPFS locator (production glob heuristics) and a fixed locator (local mode and tests).
3. Filesystem calls in request paths run off the event loop with timeouts. A hung GPFS mount degrades one request, not the whole server.
4. A failed resolution raises `ProposalNotFoundError` (see [ADR-001](001-error-classes.md)) at the edge, not a bare error from a constructor.
4 changes: 2 additions & 2 deletions api/docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,13 @@ For more information, see [ADR-000](adr/000-vertical-slice-architecture.md).
| Package | Capability | What belongs there | Status | Today |
| --------------------------------- | --------------------------------------- | ------------------------------------------------------------------------------------------------------------------------- | ------- | ------------------------------------------------------------------------- |
| `runs/` | Run/variable data - the core domain | Domain models, repository interface + implementations, serialisation, preview extraction, its GraphQL types and resolvers | Planned | `db.py` + `data.py`; resolvers in `graphql/queries.py`/`subscriptions.py` |
| `proposals/` | Proposal metadata and lookup | Proposal models, MyMdC-backed metadata services, path locator | Planned | `metadata/` |
| `proposals/` | Proposal metadata and lookup | Proposal models, MyMdC-backed metadata services, path locator (see [ADR-004](adr/004-proposal-path-locator.md)) | Planned | `metadata/` |
| `auth/` | Authentication and authorisation | OAuth flow, sessions, token store, `User`, permission classes, the membership policy | Partial | Policy still in `metadata/services.py` |
| `contextfile/` | Context-file viewing | File reading, watching, its routes | Done | As-is |
| `graphql/` | GraphQL transport only | Schema assembly, context, directives, controller binding - no resolvers, no domain logic | Partial | Assembly still in `shared/gql.py`; resolvers still here |
| `appdb/` | The app's own database (infrastructure) | Models, engine/session plumbing for `dw_api.sqlite` | Planned | `_db/` |
| `mymdc/` | MyMdC client (infrastructure) | Ports, clients, vendored models | Planned | `_mymdc/` |
| `core/` | Cross-cutting, framework-free | Shared error classes (see [ADR-001](adr/001-error-classes.md)), `DamnitType`, value types, converters | Planned | `shared/` + `utils.py` |
| `core/` | Cross-cutting, framework-free | Shared error classes (see [ADR-001](adr/001-error-classes.md)), `DamnitType`, value types (see [ADR-004](adr/004-proposal-path-locator.md)), converters | Planned | `shared/` + `utils.py` |
| `main.py` / `app.py` / `state.py` | Composition root | `AppState`, `create_*` factories (see [ADR-002](adr/002-no-global-mutable-state.md)), `create_app()` - the only place that may import everything and read settings | Partial | `main.py` + `state.py` |

Where new code goes:
Expand Down
6 changes: 5 additions & 1 deletion api/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ test = [
"pytest-recording>=0.13.4",
"syrupy>=5.5.2",
]
lint = ["pyright>=1.1.406", "ruff>=0.7"]
lint = [
"pyright>=1.1.406",
"ruff>=0.7",
"ty>=0.0.42",

@CammilleCC CammilleCC Jul 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use pyright AND ty? Might make more sense to choose one over the other. Also please include the chosen type checker in the CI and pre-commit.

]
docs = [
"zensical>=0.0.47",
"mkdocstrings-python>=2.0.3",
Expand Down
4 changes: 2 additions & 2 deletions api/src/damnit_api/_mymdc/clients.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def __init__(self, auth: MyMdCAuth) -> None:
super().__init__(auth=auth, base_url=api_url)

async def _get_proposal_by_number(self, no: models.ProposalNumber):
response = await self.get(f"proposals/by_number/{no}")
response = await self.get(f"proposals/by_number/{no:d}")
response.raise_for_status()
return response.json()

Expand Down Expand Up @@ -179,7 +179,7 @@ async def _replay(self, path: str) -> dict:
return orjson.loads(body)

async def _get_proposal_by_number(self, no: models.ProposalNumber):
return await self._replay(f"proposals/by_number/{no}")
return await self._replay(f"proposals/by_number/{no:d}")

async def _get_user_by_id(self, id: models.UserId):
return await self._replay(f"users/{id}")
Expand Down
5 changes: 4 additions & 1 deletion api/src/damnit_api/auth/gql.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from ..metadata.gql import ProposalMeta
from ..metadata.services import _get_proposal_meta_many
from ..shared.models import ProposalNumber
from .models import OAuthUserInfo

if TYPE_CHECKING:
Expand Down Expand Up @@ -43,7 +44,9 @@ async def proposals(

proposals = await mymdc.get_user_proposals(self.preferred_username)
proposal_numbers = [
p.proposal_number for p in proposals.root if p.proposal_number is not None
ProposalNumber(p.proposal_number)
for p in proposals.root
if p.proposal_number is not None
]

proposals_meta = await _get_proposal_meta_many(
Expand Down
11 changes: 7 additions & 4 deletions api/src/damnit_api/auth/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from .. import get_logger
from .._db.dependencies import DBSession
from .._mymdc.dependencies import MyMdCClient
from ..shared.models import ProposalNumber

logger = get_logger()

Expand Down Expand Up @@ -64,17 +65,17 @@ def from_connection(cls, connection: HTTPConnection) -> Self:
class ProposalsByYearHalf(RootModel):
"""Mapping of year half (e.g. 202401, 202402) to list of proposal numbers."""

root: dict[int, list[int]]
root: dict[int, list[ProposalNumber]]


class User(BaseUserInfo):
"""Full user information including list of proposals."""

_member_proposals: list[int] = PrivateAttr(default_factory=list)
_member_proposals: list[ProposalNumber] = PrivateAttr(default_factory=list)
proposals_by_year_half: ProposalsByYearHalf

@property
def proposals(self) -> list[int]:
def proposals(self) -> list[ProposalNumber]:
"""Proposals the user is a member of (raw MyMdC membership)."""
return self._member_proposals

Expand Down Expand Up @@ -113,7 +114,9 @@ async def from_oauth_user(

proposals = await mymdc.get_user_proposals(oauth.preferred_username)
member_proposals = [
p.proposal_number for p in proposals.root if p.proposal_number is not None
ProposalNumber(p.proposal_number)
for p in proposals.root
if p.proposal_number is not None
]

proposals_meta = await _get_proposal_meta_many(
Expand Down
15 changes: 3 additions & 12 deletions api/src/damnit_api/auth/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,17 @@ async def has_permission(self, source, info: Info, **kwargs) -> bool:
msg = "Field is misconfigured for proposal authorization."
raise StrawberryGraphQLError(msg)

proposal_str = getattr(kwargs["database"], "proposal", None)
if not proposal_str:
proposal = getattr(kwargs["database"], "proposal", None)
if not proposal:
return False

try:
proposal = int(proposal_str.strip("p"))
except (ValueError, TypeError):
logger.info("Invalid proposal identifier", proposal=proposal_str)
# NOTE: Strawberry shares one permission instance across all requests, so a
# per-call message must be raised, not stored on `self`.
msg = "Invalid proposal identifier."
raise StrawberryGraphQLError(msg) from None

try:
user = await info.context.get_user()
except Exception as exc:
# Do not respond with upstream errors directly, might contain internal
# info that shouldn't be sent to client.
msg = "Could not verify proposal access"
logger.exception(msg, proposal=proposal_str)
logger.exception(msg, proposal=proposal)
raise StrawberryGraphQLError(msg) from exc

try:
Expand Down
14 changes: 5 additions & 9 deletions api/src/damnit_api/graphql/queries.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from ..runs.preview import get_preview_data
from ..runs.sqlite import async_table, get_session
from ..runs.types import KNOWN_DTYPES, DamnitRun
from ..shared.models import ProposalNumber
from .metadata import fetch_metadata
from .utils import DatabaseInput, fetch_info

Expand All @@ -20,7 +21,7 @@
RUN_INFO_NAMES = frozenset(KNOWN_DTYPES) - {"proposal", "run"}


async def _ensure_damnit_path(info: Info, proposal: str) -> None:
async def _ensure_damnit_path(info: Info, proposal: ProposalNumber) -> None:
"""Ensure the proposal has a DAMNIT path, refreshing from MyMdC if needed.

Authorization is handled separately by IsProposalMember before this runs.
Expand All @@ -31,12 +32,12 @@ async def _ensure_damnit_path(info: Info, proposal: str) -> None:
return

meta = await _get_proposal_meta(
info.context.mymdc, int(proposal), info.context.session
info.context.mymdc, proposal, info.context.session
)
if not meta.damnit_path:
logger.info("No damnit path found, updating proposal metadata")
meta = await _update_proposal_meta(
info.context.mymdc, int(proposal), info.context.session
info.context.mymdc, proposal, info.context.session
)
if not meta.damnit_path:
msg = "No damnit path found after updating proposal metadata."
Expand Down Expand Up @@ -210,11 +211,6 @@ async def metadata(
database: DatabaseInput,
) -> JSON: # FIX: # pyright: ignore[reportInvalidTypeForm]
proposal = database.proposal
if not proposal:
msg = "Proposal number is required."
# TODO: custom exceptions
raise ValueError(msg)

await _ensure_damnit_path(info, proposal)

snapshot = await fetch_metadata(info.context.damnit_registry, proposal)
Expand All @@ -235,7 +231,7 @@ async def extracted_data(
# TODO: Convert to Strawberry type
# and make it analogous to DamitVariable; e.g. `data`
return get_preview_data( # FIX: # pyright: ignore[reportReturnType]
proposal=database.proposal,
proposal_number=database.proposal,
run=run,
variable=variable,
)
5 changes: 4 additions & 1 deletion api/src/damnit_api/graphql/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@

from ..runs.sqlite import DamnitDBRegistry, async_table, get_session
from ..shared.const import DEFAULT_PROPOSAL
from ..shared.models import ProposalNumber


@strawberry.input
class DatabaseInput:
proposal: str | None = strawberry.field(default=DEFAULT_PROPOSAL)
proposal: ProposalNumber = strawberry.field(
default=ProposalNumber(DEFAULT_PROPOSAL)
)
path: str | None = strawberry.field(default=strawberry.UNSET)


Expand Down
5 changes: 4 additions & 1 deletion api/src/damnit_api/metadata/gql.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@

from .. import get_logger
from ..auth.permissions import IsAuthenticated
from ..shared.models import (
ProposalNumber, # noqa: TC001 (Strawberry resolves at runtime)
)
from . import models, services

if TYPE_CHECKING:
Expand Down Expand Up @@ -37,7 +40,7 @@ class Query:
async def proposal_metadata(
self,
info: strawberry.Info[Context],
proposal_numbers: list[int],
proposal_numbers: list[ProposalNumber],
) -> list[ProposalMeta] | None:
"""Fetch metadata for the given proposal numbers.

Expand Down
9 changes: 5 additions & 4 deletions api/src/damnit_api/metadata/services.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,24 @@ def _local_proposal_meta(proposal_number: ProposalNumber) -> ProposalMeta:
)


async def _local_proposal_number(registry: "DamnitDBRegistry") -> int | None:
async def _local_proposal_number(registry: "DamnitDBRegistry") -> ProposalNumber | None:
from sqlalchemy import select

from ..runs.sqlite import async_table, get_session
from ..shared.const import DEFAULT_PROPOSAL

table = await async_table(registry, DEFAULT_PROPOSAL, name="metameta")
proposal = ProposalNumber(DEFAULT_PROPOSAL)
table = await async_table(registry, proposal, name="metameta")
if table is None:
return None

async with get_session(registry, DEFAULT_PROPOSAL) as session:
async with get_session(registry, proposal) as session:
result = await session.execute(
select(table.c.value).where(table.c.key == "proposal")
)
value = result.scalar()

return int(value) if value else None
return ProposalNumber(int(value)) if value else None


async def _fetch_proposal_meta(
Expand Down
5 changes: 3 additions & 2 deletions api/src/damnit_api/runs/preview.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
from PIL import Image

from ..shared.const import DamnitType
from ..shared.models import ProposalNumber
from ..utils import b64image
from .sqlite import get_damnit_path

NOT_SUPPORTED_MESSAGE = "Not supported."


def get_preview_data(proposal, run, variable):
path = get_damnit_path(str(proposal))
def get_preview_data(proposal_number: ProposalNumber, run, variable):
path = get_damnit_path(proposal_number)
try:
var_data = Damnit(path)[run, variable]
except KeyError:
Expand Down
19 changes: 11 additions & 8 deletions api/src/damnit_api/runs/sqlite/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,20 @@
from sqlalchemy.pool import NullPool

from ...shared.const import DEFAULT_PROPOSAL
from ...shared.errors import ProposalNotFoundError
from ...shared.models import ProposalNumber
from ...utils import find_proposal

DAMNIT_PATH = "usr/Shared/amore/"
_DEFAULT_PROPOSAL = ProposalNumber(DEFAULT_PROPOSAL)


# -----------------------------------------------------------------------------
# Asynchronous


class DatabaseSessionManager:
def __init__(self, proposal: str = DEFAULT_PROPOSAL):
def __init__(self, proposal: ProposalNumber = _DEFAULT_PROPOSAL):
self.proposal = proposal
self.root_path = get_damnit_path(proposal)
self._engine = create_async_engine(
Expand Down Expand Up @@ -97,16 +100,16 @@ class DamnitDBRegistry:
"""Per-proposal DAMNIT database registry."""

def __init__(self) -> None:
self._managers: dict[str, DatabaseSessionManager] = {}
self._managers: dict[ProposalNumber, DatabaseSessionManager] = {}

def get(self, proposal: str) -> DatabaseSessionManager:
def get(self, proposal: ProposalNumber) -> DatabaseSessionManager:
if proposal not in self._managers:
self._managers[proposal] = DatabaseSessionManager(proposal)
return self._managers[proposal]


def get_session(
registry: DamnitDBRegistry, proposal: str
registry: DamnitDBRegistry, proposal: ProposalNumber
) -> AbstractAsyncContextManager[AsyncSession]:
return registry.get(proposal).session()

Expand All @@ -115,15 +118,15 @@ def get_session(
# Etc.


def get_damnit_path(proposal_number: str = DEFAULT_PROPOSAL) -> str:
def get_damnit_path(proposal: ProposalNumber = _DEFAULT_PROPOSAL) -> str:
"""Returns the directory of the given proposal."""
from ...shared.settings import settings

if settings.is_local:
return str(settings.damnit_path)

path = find_proposal(proposal_number)
path = find_proposal(proposal)
if not path:
msg = f"Proposal '{proposal_number}' is not found."
raise RuntimeError(msg)
msg = f"Proposal '{proposal}' is not found."
raise ProposalNotFoundError(msg)
return str(Path(path) / DAMNIT_PATH)
Loading
Loading