From 811869e58005a6a4365dbdbb9f258a24857fb3a9 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Mon, 16 Mar 2026 14:57:51 +1100 Subject: [PATCH 1/5] refactor: use selectinload to prefetch memberships in user queries --- routers/admin.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/routers/admin.py b/routers/admin.py index 148709f2..6987d04a 100644 --- a/routers/admin.py +++ b/routers/admin.py @@ -10,6 +10,7 @@ from pydantic import BaseModel, Field, ValidationError, field_validator from sqlalchemy import func, or_ from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import selectinload from sqlmodel import Session, select from sqlmodel.sql._expression_select_cls import SelectOfScalar from starlette import status @@ -360,6 +361,9 @@ def get_base_query(self): """ return ( select(BiocommonsUser) + .options( + selectinload(BiocommonsUser.platform_memberships).selectinload(PlatformMembership.platform), + selectinload(BiocommonsUser.group_memberships).selectinload(GroupMembership.group),) ) def _set_allowed_resource_subqueries(self, admin_roles: list[str]) -> None: From 310182264f4773f864d3d4ecad8fda6a15e05945 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Mon, 16 Mar 2026 15:03:01 +1100 Subject: [PATCH 2/5] refactor: use exists() queries for better performance --- routers/admin.py | 101 +++++++++++++++++++++++++++++------------------ 1 file changed, 63 insertions(+), 38 deletions(-) diff --git a/routers/admin.py b/routers/admin.py index 6987d04a..6b8691f8 100644 --- a/routers/admin.py +++ b/routers/admin.py @@ -8,7 +8,7 @@ from fastapi.params import Query from httpx import HTTPStatusError from pydantic import BaseModel, Field, ValidationError, field_validator -from sqlalchemy import func, or_ +from sqlalchemy import exists, func, or_ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import selectinload from sqlmodel import Session, select @@ -396,14 +396,18 @@ def get_admin_permissions_query(self, admin_roles: list[str]): based on group/platform roles """ allowed_platforms_subquery, allowed_groups_subquery = self.get_allowed_resource_subqueries(admin_roles) - platform_access_condition = BiocommonsUser.id.in_( - select(PlatformMembership.user_id).where( - PlatformMembership.platform_id.in_(allowed_platforms_subquery) + platform_access_condition = exists( + select(1).where( + PlatformMembership.user_id == BiocommonsUser.id, + PlatformMembership.platform_id.in_(allowed_platforms_subquery), + PlatformMembership.is_deleted.is_(False), ) ) - group_access_condition = BiocommonsUser.id.in_( - select(GroupMembership.user_id).where( - GroupMembership.group_id.in_(allowed_groups_subquery) + group_access_condition = exists( + select(1).where( + GroupMembership.user_id == BiocommonsUser.id, + GroupMembership.group_id.in_(allowed_groups_subquery), + GroupMembership.is_deleted.is_(False), ) ) return or_(platform_access_condition, group_access_condition) @@ -482,30 +486,36 @@ def platform_query(self): so we need special logic for combining them. :return: """ - conditions = [PlatformMembership.platform_id == self.platform] + conditions = [ + PlatformMembership.user_id == BiocommonsUser.id, + PlatformMembership.platform_id == self.platform, + PlatformMembership.is_deleted.is_(False), + ] if self.platform_approval_status is not None: conditions.append(PlatformMembership.approval_status == self.platform_approval_status) - platform_query = select(PlatformMembership.user_id).where(*conditions) - return BiocommonsUser.id.in_(platform_query) + return exists(select(1).where(*conditions)) def platform_approval_status_query(self): # If platform is set, let platform_query handle the combined query if self.platform is not None: return None - platform_status_query = select(PlatformMembership.user_id).where( - PlatformMembership.approval_status == self.platform_approval_status - ) + conditions = [ + PlatformMembership.user_id == BiocommonsUser.id, + PlatformMembership.approval_status == self.platform_approval_status, + PlatformMembership.is_deleted.is_(False), + ] if self._allowed_platforms_subquery is not None: - platform_status_query = platform_status_query.where( - PlatformMembership.platform_id.in_(self._allowed_platforms_subquery) - ) - return BiocommonsUser.id.in_(platform_status_query) + conditions.append(PlatformMembership.platform_id.in_(self._allowed_platforms_subquery)) + return exists(select(1).where(*conditions)) def group_query(self): - group_query = select(GroupMembership.user_id).where( - GroupMembership.group_id == self.group + return exists( + select(1).where( + GroupMembership.user_id == BiocommonsUser.id, + GroupMembership.group_id == self.group, + GroupMembership.is_deleted.is_(False), + ) ) - return BiocommonsUser.id.in_(group_query) def group_approval_status_query(self): """ @@ -514,10 +524,13 @@ def group_approval_status_query(self): already enforces visibility. That allows platform admins (who may not be group admins) to still see group-status results for the users they manage. """ - group_status_query = select(GroupMembership.user_id).where( - GroupMembership.approval_status == self.group_approval_status + return exists( + select(1).where( + GroupMembership.user_id == BiocommonsUser.id, + GroupMembership.approval_status == self.group_approval_status, + GroupMembership.is_deleted.is_(False), + ) ) - return BiocommonsUser.id.in_(group_status_query) def approval_status_query(self, admin_roles: list[str] | None = None): """ @@ -528,17 +541,22 @@ def approval_status_query(self, admin_roles: list[str] | None = None): raise ValueError("Allowed resource subqueries must be set before calling approval_status_query") self._set_allowed_resource_subqueries(admin_roles) - platform_status_query = select(PlatformMembership.user_id).where( - PlatformMembership.platform_id.in_(self._allowed_platforms_subquery), - PlatformMembership.approval_status == self.approval_status, - ) - group_status_query = select(GroupMembership.user_id).where( - GroupMembership.approval_status == self.approval_status, + platform_status_exists = exists( + select(1).where( + PlatformMembership.user_id == BiocommonsUser.id, + PlatformMembership.platform_id.in_(self._allowed_platforms_subquery), + PlatformMembership.approval_status == self.approval_status, + PlatformMembership.is_deleted.is_(False), + ) ) - return or_( - BiocommonsUser.id.in_(platform_status_query), - BiocommonsUser.id.in_(group_status_query), + group_status_exists = exists( + select(1).where( + GroupMembership.user_id == BiocommonsUser.id, + GroupMembership.approval_status == self.approval_status, + GroupMembership.is_deleted.is_(False), + ) ) + return or_(platform_status_exists, group_status_exists) def get_count(self, db_session: Session, admin_roles: list[str]) -> int: """ @@ -568,16 +586,22 @@ def search_query(self): def filter_by_query(self): if self.filter_by in GROUP_MAPPING: full_group_id = GROUP_MAPPING[self.filter_by]["enum"].value - group_subquery = select(GroupMembership.user_id).where( - GroupMembership.group_id == full_group_id + return exists( + select(1).where( + GroupMembership.user_id == BiocommonsUser.id, + GroupMembership.group_id == full_group_id, + GroupMembership.is_deleted.is_(False), + ) ) - return BiocommonsUser.id.in_(group_subquery) elif self.filter_by in PLATFORM_MAPPING: platform_enum_value = PLATFORM_MAPPING[self.filter_by]["enum"] - platform_subquery = select(PlatformMembership.user_id).where( - PlatformMembership.platform_id == platform_enum_value + return exists( + select(1).where( + PlatformMembership.user_id == BiocommonsUser.id, + PlatformMembership.platform_id == platform_enum_value, + PlatformMembership.is_deleted.is_(False), + ) ) - return BiocommonsUser.id.in_(platform_subquery) else: raise HTTPException( status_code=400, @@ -585,6 +609,7 @@ def filter_by_query(self): ) + def get_filtered_user_query( admin_user: Annotated[SessionUser, Depends(get_session_user)], user_query: Annotated[UserQueryParams, Depends()], From 1310eef66196845438041ebfe5c92ffa553a79df Mon Sep 17 00:00:00 2001 From: marius-mather Date: Tue, 31 Mar 2026 09:23:42 +1100 Subject: [PATCH 3/5] fix: load membership.updated_by with selectinload() --- routers/admin.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/routers/admin.py b/routers/admin.py index 6b8691f8..df955ea1 100644 --- a/routers/admin.py +++ b/routers/admin.py @@ -363,7 +363,10 @@ def get_base_query(self): select(BiocommonsUser) .options( selectinload(BiocommonsUser.platform_memberships).selectinload(PlatformMembership.platform), - selectinload(BiocommonsUser.group_memberships).selectinload(GroupMembership.group),) + selectinload(BiocommonsUser.platform_memberships).selectinload(PlatformMembership.updated_by), + selectinload(BiocommonsUser.group_memberships).selectinload(GroupMembership.group), + selectinload(BiocommonsUser.group_memberships).selectinload(GroupMembership.updated_by), + ) ) def _set_allowed_resource_subqueries(self, admin_roles: list[str]) -> None: From 506d0e38143c1cea59df77b6c5526df982848368 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Tue, 31 Mar 2026 09:26:00 +1100 Subject: [PATCH 4/5] fix: make sure group status query filters on allowed groups --- routers/admin.py | 1 + 1 file changed, 1 insertion(+) diff --git a/routers/admin.py b/routers/admin.py index df955ea1..01951510 100644 --- a/routers/admin.py +++ b/routers/admin.py @@ -555,6 +555,7 @@ def approval_status_query(self, admin_roles: list[str] | None = None): group_status_exists = exists( select(1).where( GroupMembership.user_id == BiocommonsUser.id, + GroupMembership.group_id.in_(self._allowed_groups_subquery), GroupMembership.approval_status == self.approval_status, GroupMembership.is_deleted.is_(False), ) From 88d0f1daa322e423012d7695988d8001f39dddb6 Mon Sep 17 00:00:00 2001 From: marius-mather Date: Tue, 31 Mar 2026 09:43:34 +1100 Subject: [PATCH 5/5] refactor: use explicit name for status code --- routers/admin.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/routers/admin.py b/routers/admin.py index 34e3c0da..59b75de4 100644 --- a/routers/admin.py +++ b/routers/admin.py @@ -1,3 +1,4 @@ +import http import inspect import logging import math @@ -350,7 +351,7 @@ def model_post_init(self, context: Any) -> None: raise NotImplementedError(f"Missing query method for field '{field_name}'") if self.approval_status and (self.platform_approval_status or self.group_approval_status): raise HTTPException( - status_code=400, + status_code=http.HTTPStatus.BAD_REQUEST, detail="approval_status cannot be used with platform_approval_status or group_approval_status", )