diff --git a/app/db/crud/group.py b/app/db/crud/group.py index 64140c0c6..04c18c81f 100644 --- a/app/db/crud/group.py +++ b/app/db/crud/group.py @@ -1,6 +1,6 @@ from sqlalchemy import delete, func, select from sqlalchemy.ext.asyncio import AsyncSession -from sqlalchemy.orm import selectinload +from sqlalchemy.orm import selectinload, with_expression from app.db.models import ( Group, @@ -42,6 +42,7 @@ async def get_inbounds_by_tags(db: AsyncSession, tags: list[str]) -> list[ProxyI async def load_group_attrs(group: Group, *, load_users: bool = True, load_inbounds: bool = True): if load_users: await group.awaitable_attrs.users + group.total_users = len(group.users) if load_inbounds: await group.awaitable_attrs.inbounds @@ -105,7 +106,16 @@ async def get_group(db: AsyncSession, query: GroupListQuery) -> tuple[list[Group - list[Group]: A list of Group objects - int: The total count of groups """ - groups = select(Group).options(selectinload(Group.users), selectinload(Group.inbounds)) + total_users = ( + select(func.count(users_groups_association.c.user_id)) + .where(users_groups_association.c.groups_id == Group.id) + .correlate(Group) + .scalar_subquery() + ) + groups = select(Group).options( + with_expression(Group.total_users, total_users), + selectinload(Group.inbounds), + ) if query.ids: groups = groups.where(Group.id.in_(query.ids)) @@ -122,7 +132,8 @@ async def get_group(db: AsyncSession, query: GroupListQuery) -> tuple[list[Group count = (await db.execute(count_query)).scalar_one() - # users and inbounds already eagerly loaded via selectinload above + # Inbounds are eagerly loaded; total_users is populated by the SQL expression + # without materializing the User relationship. all_groups = (await db.execute(groups)).unique().scalars().all() return all_groups, count diff --git a/app/db/migrations/versions/a4d8c7e91b32_add_reverse_group_membership_indexes.py b/app/db/migrations/versions/a4d8c7e91b32_add_reverse_group_membership_indexes.py new file mode 100644 index 000000000..0c7a5af82 --- /dev/null +++ b/app/db/migrations/versions/a4d8c7e91b32_add_reverse_group_membership_indexes.py @@ -0,0 +1,72 @@ +"""add reverse group membership indexes + +Revision ID: a4d8c7e91b32 +Revises: 6a9fff8290b9 +Create Date: 2026-08-14 15:00:00.000000 + +""" + +from alembic import op + + +# revision identifiers, used by Alembic. +revision = "a4d8c7e91b32" +down_revision = "6a9fff8290b9" +branch_labels = None +depends_on = None + + +INDEXES = ( + ( + "ix_inbounds_groups_association_group_id_inbound_id", + "inbounds_groups_association", + ["group_id", "inbound_id"], + ), + ( + "ix_users_groups_association_groups_id_user_id", + "users_groups_association", + ["groups_id", "user_id"], + ), +) + + +def _create_indexes(*, concurrently: bool) -> None: + for name, table_name, columns in INDEXES: + op.create_index( + name, + table_name, + columns, + unique=False, + postgresql_concurrently=concurrently, + ) + + +def _drop_indexes(*, concurrently: bool) -> None: + for name, table_name, _ in reversed(INDEXES): + op.drop_index( + name, + table_name=table_name, + postgresql_concurrently=concurrently, + ) + + +def upgrade() -> None: + if op.get_bind().dialect.name == "postgresql": + # A normal PostgreSQL index build blocks membership writes. These tables + # can contain millions of rows, so keep live installations writable. + with op.get_context().autocommit_block(): + _create_indexes(concurrently=True) + return + + # InnoDB creates secondary indexes online by default. SQLite needs the + # regular form and is normally used for smaller, single-node deployments. + _create_indexes(concurrently=False) + + +def downgrade() -> None: + if op.get_bind().dialect.name == "postgresql": + with op.get_context().autocommit_block(): + _drop_indexes(concurrently=True) + return + + _drop_indexes(concurrently=False) diff --git a/app/db/models.py b/app/db/models.py index a3bb13e92..a8200ef14 100644 --- a/app/db/models.py +++ b/app/db/models.py @@ -25,7 +25,7 @@ from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.ext.asyncio import async_object_session from sqlalchemy.ext.hybrid import hybrid_property -from sqlalchemy.orm import Mapped, mapped_column, relationship +from sqlalchemy.orm import Mapped, mapped_column, query_expression, relationship from sqlalchemy.sql.expression import select, text from app.db.base import Base @@ -58,6 +58,20 @@ def fk_id_table_column(name: str, target: str, **column_kwargs: Any): fk_id_table_column("groups_id", "groups.id", primary_key=True), ) +# The association primary keys are ordered from the owning entity to the group. +# Reverse indexes keep group-centric joins, counts, and bulk deletes from scanning +# every membership row on databases that do not auto-index foreign keys. +Index( + "ix_inbounds_groups_association_group_id_inbound_id", + inbounds_groups_association.c.group_id, + inbounds_groups_association.c.inbound_id, +) +Index( + "ix_users_groups_association_groups_id_user_id", + users_groups_association.c.groups_id, + users_groups_association.c.user_id, +) + class AdminStatus(str, Enum): active = "active" @@ -771,6 +785,7 @@ class Group(Base, IdMixin): secondary=template_group_association, back_populates="groups", init=False ) is_disabled: Mapped[bool] = mapped_column(server_default="0", default=False) + total_users: Mapped[int] = query_expression(repr=False) @hybrid_property def inbound_ids(self) -> list[int]: @@ -802,19 +817,6 @@ def inbound_tags(cls): .label("inbound_tags") ) - @hybrid_property - def total_users(self) -> int: - return len(self.users) - - @total_users.expression - def total_users(cls): - return ( - select(func.count(users_groups_association.c.user_id)) - .where(users_groups_association.c.groups_id == cls.id) - .scalar_subquery() - .label("total_users") - ) - class CoreType(str, Enum): xray = "xray" diff --git a/tests/api/test_group.py b/tests/api/test_group.py index 38cafbb0b..2ad522276 100644 --- a/tests/api/test_group.py +++ b/tests/api/test_group.py @@ -1,9 +1,23 @@ +import asyncio import random from fastapi import status - -from tests.api import client -from tests.api.helpers import create_core, create_group, delete_core, delete_group, get_inbounds, unique_name +from sqlalchemy import inspect +from sqlalchemy.orm.state import NO_VALUE + +from app.db.crud.group import get_group +from app.models.group import GroupListQuery +from tests.api import TestSession, client +from tests.api.helpers import ( + create_core, + create_group, + create_user, + delete_core, + delete_group, + delete_user, + get_inbounds, + unique_name, +) def test_group_create(access_token): @@ -91,6 +105,31 @@ def test_groups_get(access_token): delete_core(access_token, core["id"]) +def test_groups_get_counts_users_without_loading_user_rows(access_token): + """Group summaries count memberships without hydrating every user.""" + + core = create_core(access_token) + group = create_group(access_token, name=unique_name("group_count")) + users = [create_user(access_token, group_ids=[group["id"]]) for _ in range(2)] + + async def load_group_summary(): + async with TestSession() as session: + groups, _ = await get_group(session, GroupListQuery(ids=[group["id"]])) + loaded_group = groups[0] + return loaded_group.total_users, inspect(loaded_group).attrs.users.loaded_value + + try: + total_users, loaded_users = asyncio.run(load_group_summary()) + + assert total_users == len(users) + assert loaded_users is NO_VALUE + finally: + for user in users: + delete_user(access_token, user["username"]) + delete_group(access_token, group["id"]) + delete_core(access_token, core["id"]) + + # Tests for /api/groups/simple endpoint diff --git a/tests/test_group_membership_indexes.py b/tests/test_group_membership_indexes.py new file mode 100644 index 000000000..535a789e4 --- /dev/null +++ b/tests/test_group_membership_indexes.py @@ -0,0 +1,25 @@ +import pytest + +from app.db.models import inbounds_groups_association, users_groups_association + + +@pytest.mark.parametrize( + ("table", "index_name", "columns"), + [ + ( + inbounds_groups_association, + "ix_inbounds_groups_association_group_id_inbound_id", + ("group_id", "inbound_id"), + ), + ( + users_groups_association, + "ix_users_groups_association_groups_id_user_id", + ("groups_id", "user_id"), + ), + ], +) +def test_group_associations_have_covering_reverse_indexes(table, index_name, columns): + index = next((candidate for candidate in table.indexes if candidate.name == index_name), None) + + assert index is not None + assert tuple(column.name for column in index.columns) == columns