From 70daf4b2ee444ef9abbe52237ebc0b025c9222fb Mon Sep 17 00:00:00 2001 From: Gyubong Date: Tue, 21 Jul 2026 14:08:16 +0900 Subject: [PATCH 1/6] refactor(BA-6948): store app_config_fragments.scope_id as a nullable UUID scope_id was never really a string: a domain fragment stores a domain id and a user fragment a user id, both UUIDs, while a public fragment has no owner and stored '' only because the column was NOT NULL. Search scopes therefore had to compare against str(domain_id), and a malformed scope_id could be persisted into a row no scope query would ever reach. Store UUID NULL instead, with NULL meaning public. Uniqueness needs restating: Postgres counts NULLs as distinct, so the existing constraint on (config_name, scope_type, scope_id) stops rejecting a second public fragment once public rows hold NULL. A partial unique index over the NULL rows restores what the '' sentinel used to guarantee. UNIQUE NULLS NOT DISTINCT would say this in one constraint but needs Postgres 15+, and the test fixture runs 13. The RBAC boundary still identifies scopes by string, so to_rbac_scope_id now takes UUID | None and renders it, empty for public. The migration forces public rows to NULL whatever they stored, and fails loudly on a domain/user scope_id that is not a UUID rather than nulling it, since that binding decides who can see the fragment. Upgrade and downgrade were both run against a local DB with a row per scope type. Co-Authored-By: Claude Opus 4.8 (1M context) --- changes/12984.enhance.md | 1 + .../backend/common/data/app_config/types.py | 8 +- .../manager/data/app_config_fragment/types.py | 3 +- ...a8_app_config_fragment_scope_id_to_uuid.py | 76 ++++++++++++++++ .../models/app_config_fragment/conditions.py | 12 ++- .../manager/models/app_config_fragment/row.py | 20 +++- .../app_config_fragment/creators.py | 3 +- .../db_source/db_source.py | 8 +- .../repositories/app_config_fragment/types.py | 4 +- .../app_config_fragment/actions/create.py | 3 +- .../app_config_fragment/test_repository.py | 91 ++++++++++--------- 11 files changed, 169 insertions(+), 60 deletions(-) create mode 100644 changes/12984.enhance.md create mode 100644 src/ai/backend/manager/models/alembic/versions/e5b71c94d2a8_app_config_fragment_scope_id_to_uuid.py diff --git a/changes/12984.enhance.md b/changes/12984.enhance.md new file mode 100644 index 00000000000..9a05d2d0ae6 --- /dev/null +++ b/changes/12984.enhance.md @@ -0,0 +1 @@ +Store `app_config_fragments.scope_id` as a nullable UUID rather than a string, with `NULL` for the ownerless public scope. diff --git a/src/ai/backend/common/data/app_config/types.py b/src/ai/backend/common/data/app_config/types.py index 0846d93caf1..d296f0b7a41 100644 --- a/src/ai/backend/common/data/app_config/types.py +++ b/src/ai/backend/common/data/app_config/types.py @@ -3,6 +3,7 @@ from __future__ import annotations import enum +import uuid from ai.backend.common.data.permission.types import RBACElementType, ScopeType @@ -49,13 +50,14 @@ def to_rbac_element_type(self) -> RBACElementType | None: case AppConfigScopeType.USER: return RBACElementType.USER - def to_rbac_scope_id(self, scope_id: str) -> str: + def to_rbac_scope_id(self, scope_id: uuid.UUID | None) -> str: """The RBAC scope id for a write at this fragment scope. ``public`` is system-wide (no per-entity scope id); ``domain`` / ``user`` carry - their own ``scope_id``. + their own ``scope_id``. RBAC identifies scopes by string, so the owner id is + rendered as text here even though it is stored as a UUID. """ - return "" if self is AppConfigScopeType.PUBLIC else scope_id + return "" if self is AppConfigScopeType.PUBLIC else str(scope_id) def default_rank(self) -> int: """Default merge rank for an allow-list entry at this scope type (BEP-1052). diff --git a/src/ai/backend/manager/data/app_config_fragment/types.py b/src/ai/backend/manager/data/app_config_fragment/types.py index cd5f451acdd..485f37c2266 100644 --- a/src/ai/backend/manager/data/app_config_fragment/types.py +++ b/src/ai/backend/manager/data/app_config_fragment/types.py @@ -1,5 +1,6 @@ from __future__ import annotations +import uuid from dataclasses import dataclass from datetime import datetime from typing import Any @@ -15,7 +16,7 @@ class AppConfigFragmentData: id: AppConfigFragmentID config_name: str scope_type: AppConfigScopeType - scope_id: str + scope_id: uuid.UUID | None config: dict[str, Any] created_at: datetime updated_at: datetime diff --git a/src/ai/backend/manager/models/alembic/versions/e5b71c94d2a8_app_config_fragment_scope_id_to_uuid.py b/src/ai/backend/manager/models/alembic/versions/e5b71c94d2a8_app_config_fragment_scope_id_to_uuid.py new file mode 100644 index 00000000000..bc2c233f86f --- /dev/null +++ b/src/ai/backend/manager/models/alembic/versions/e5b71c94d2a8_app_config_fragment_scope_id_to_uuid.py @@ -0,0 +1,76 @@ +"""convert app_config_fragments.scope_id to a nullable UUID + +``scope_id`` held a ``VARCHAR(255)`` that was never really a string: a domain +fragment stores a domain id and a user fragment a user id, both UUIDs, while a +public fragment has no owner at all and stored ``''`` only because the column +was ``NOT NULL``. Store the real type instead — ``UUID NULL``, with ``NULL`` +meaning "public, no owner". + +The uniqueness of ``(config_name, scope_type, scope_id)`` needs help: Postgres +treats ``NULL``s as distinct in a unique constraint, so the existing constraint +stops rejecting a second public fragment for the same config name once public +rows hold ``NULL``. A partial unique index over the ``NULL`` rows restores the +guarantee the ``''`` sentinel used to provide. (``UNIQUE NULLS NOT DISTINCT`` +would say the same thing in one constraint, but it needs Postgres 15+ and the +test fixture runs 13.) + +Public rows are forced to ``NULL`` regardless of what they stored, since public +has no owner by definition. Domain and user rows are cast, and a value that is +not a UUID fails the migration on purpose — nulling it would silently drop the +scope binding that decides who can see the fragment. + +Revision ID: e5b71c94d2a8 +Revises: 577c7a215934 +Create Date: 2026-07-21 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "e5b71c94d2a8" +down_revision = "577c7a215934" +# Part of: NEXT_RELEASE_VERSION +branch_labels = None +depends_on = None + +_PUBLIC_INDEX = "uq_app_config_fragments_public_config_name" +_TABLE = "app_config_fragments" + + +def upgrade() -> None: + op.execute( + sa.text(""" + ALTER TABLE app_config_fragments + ALTER COLUMN scope_id DROP NOT NULL, + ALTER COLUMN scope_id TYPE UUID + USING ( + CASE + WHEN scope_type = 'public' THEN NULL + ELSE NULLIF(scope_id, '')::uuid + END + ) + """) + ) + op.create_index( + _PUBLIC_INDEX, + _TABLE, + ["config_name", "scope_type"], + unique=True, + postgresql_where=sa.text("scope_id IS NULL"), + ) + + +def downgrade() -> None: + op.drop_index(_PUBLIC_INDEX, table_name=_TABLE) + # Public rows go back to the empty sentinel the NOT NULL column used to require, which + # the plain constraint can compare like any other value. + op.execute( + sa.text(""" + ALTER TABLE app_config_fragments + ALTER COLUMN scope_id TYPE VARCHAR(255) + USING (COALESCE(scope_id::text, '')) + """) + ) + op.alter_column(_TABLE, "scope_id", nullable=False) diff --git a/src/ai/backend/manager/models/app_config_fragment/conditions.py b/src/ai/backend/manager/models/app_config_fragment/conditions.py index a8f4d836c13..37c55b60ab2 100644 --- a/src/ai/backend/manager/models/app_config_fragment/conditions.py +++ b/src/ai/backend/manager/models/app_config_fragment/conditions.py @@ -10,6 +10,8 @@ from ai.backend.common.data.app_config.types import AppConfigScopeType from ai.backend.common.data.filter_specs import StringMatchSpec +from ai.backend.common.identifier.domain import DomainID +from ai.backend.common.identifier.user import UserID from ai.backend.manager.models.app_config_fragment.row import AppConfigFragmentRow from ai.backend.manager.models.clauses import QueryCondition from ai.backend.manager.models.condition_utils import make_string_in_factory @@ -118,7 +120,7 @@ def inner() -> sa.sql.expression.ColumnElement[bool]: # --- scope_id filter --- @staticmethod - def by_scope_id_equals(scope_id: str) -> QueryCondition: + def by_scope_id_equals(scope_id: uuid.UUID) -> QueryCondition: def inner() -> sa.sql.expression.ColumnElement[bool]: return AppConfigFragmentRow.scope_id == scope_id @@ -136,19 +138,19 @@ def inner() -> sa.sql.expression.ColumnElement[bool]: return inner @staticmethod - def by_domain_visibility(domain: str) -> QueryCondition: - """The ``domain`` scope for ``domain``.""" + def by_domain_visibility(domain_id: DomainID) -> QueryCondition: + """The ``domain`` scope for ``domain_id``.""" def inner() -> sa.sql.expression.ColumnElement[bool]: return sa.and_( AppConfigFragmentRow.scope_type == AppConfigScopeType.DOMAIN, - AppConfigFragmentRow.scope_id == domain, + AppConfigFragmentRow.scope_id == domain_id, ) return inner @staticmethod - def by_user_visibility(user_id: str) -> QueryCondition: + def by_user_visibility(user_id: UserID) -> QueryCondition: """The ``user`` scope for ``user_id``.""" def inner() -> sa.sql.expression.ColumnElement[bool]: diff --git a/src/ai/backend/manager/models/app_config_fragment/row.py b/src/ai/backend/manager/models/app_config_fragment/row.py index 0a324f10652..352e491038b 100644 --- a/src/ai/backend/manager/models/app_config_fragment/row.py +++ b/src/ai/backend/manager/models/app_config_fragment/row.py @@ -1,5 +1,6 @@ from __future__ import annotations +import uuid from typing import Any import sqlalchemy as sa @@ -32,6 +33,17 @@ class AppConfigFragmentRow(LifecycleTimestampsMixin, Base): # type: ignore[misc "scope_id", name="uq_app_config_fragments_config_name_scope_type_scope_id", ), + # The constraint above only covers domain and user fragments: Postgres counts NULLs + # as distinct, so it would let a config take any number of public fragments. This + # partial index restores that guarantee for the NULL (public) rows. It replaces the + # NULLS NOT DISTINCT the constraint would otherwise want, which needs Postgres 15+. + sa.Index( + "uq_app_config_fragments_public_config_name", + "config_name", + "scope_type", + unique=True, + postgresql_where=sa.text("scope_id IS NULL"), + ), sa.ForeignKeyConstraint( ["config_name", "scope_type"], ["app_config_allow_list.config_name", "app_config_allow_list.scope_type"], @@ -56,10 +68,12 @@ class AppConfigFragmentRow(LifecycleTimestampsMixin, Base): # type: ignore[misc StrEnumType(AppConfigScopeType), nullable=False, ) - scope_id: Mapped[str] = mapped_column( + # NULL is the public scope: it has no owner. Domain and user fragments carry the id of + # the domain or user that owns them. + scope_id: Mapped[uuid.UUID | None] = mapped_column( "scope_id", - sa.String(length=255), - nullable=False, + GUID, + nullable=True, ) config: Mapped[dict[str, Any]] = mapped_column( "config", diff --git a/src/ai/backend/manager/repositories/app_config_fragment/creators.py b/src/ai/backend/manager/repositories/app_config_fragment/creators.py index 5e4c47b9d96..43a0e53173a 100644 --- a/src/ai/backend/manager/repositories/app_config_fragment/creators.py +++ b/src/ai/backend/manager/repositories/app_config_fragment/creators.py @@ -2,6 +2,7 @@ from __future__ import annotations +import uuid from collections.abc import Sequence from dataclasses import dataclass from typing import Any, override @@ -23,7 +24,7 @@ class AppConfigFragmentCreatorSpec(CreatorSpec[AppConfigFragmentRow]): config_name: str scope_type: AppConfigScopeType - scope_id: str + scope_id: uuid.UUID | None config: dict[str, Any] @property diff --git a/src/ai/backend/manager/repositories/app_config_fragment/db_source/db_source.py b/src/ai/backend/manager/repositories/app_config_fragment/db_source/db_source.py index a87ccd80f38..84ecb8ad724 100644 --- a/src/ai/backend/manager/repositories/app_config_fragment/db_source/db_source.py +++ b/src/ai/backend/manager/repositories/app_config_fragment/db_source/db_source.py @@ -85,7 +85,9 @@ async def create(self, spec: AppConfigFragmentCreatorSpec) -> AppConfigFragmentD spec=spec, element_type=RBACElementType.APP_CONFIG_FRAGMENT, scope_ref=( - RBACElementRef(element_type, spec.scope_id) if element_type is not None else None + RBACElementRef(element_type, str(spec.scope_id)) + if element_type is not None + else None ), ) async with self._rbac_ops_provider.write_ops() as w: @@ -223,8 +225,8 @@ async def list_visible_fragments_bulk( scope_visibility = [AppConfigFragmentConditions.by_public_visibility()] if scope is not None: scope_visibility += [ - AppConfigFragmentConditions.by_domain_visibility(str(scope.domain_id)), - AppConfigFragmentConditions.by_user_visibility(str(scope.user_id)), + AppConfigFragmentConditions.by_domain_visibility(scope.domain_id), + AppConfigFragmentConditions.by_user_visibility(scope.user_id), ] querier = BatchQuerier( pagination=NoPagination(), diff --git a/src/ai/backend/manager/repositories/app_config_fragment/types.py b/src/ai/backend/manager/repositories/app_config_fragment/types.py index dc982d26eda..bab654a67c9 100644 --- a/src/ai/backend/manager/repositories/app_config_fragment/types.py +++ b/src/ai/backend/manager/repositories/app_config_fragment/types.py @@ -63,7 +63,7 @@ def to_condition(self) -> QueryCondition: def inner() -> sa.sql.expression.ColumnElement[bool]: return sa.and_( AppConfigFragmentRow.scope_type == AppConfigScopeType.DOMAIN, - AppConfigFragmentRow.scope_id == str(domain_id), + AppConfigFragmentRow.scope_id == domain_id, ) return inner @@ -87,7 +87,7 @@ def to_condition(self) -> QueryCondition: def inner() -> sa.sql.expression.ColumnElement[bool]: return sa.and_( AppConfigFragmentRow.scope_type == AppConfigScopeType.USER, - AppConfigFragmentRow.scope_id == str(user_id), + AppConfigFragmentRow.scope_id == user_id, ) return inner diff --git a/src/ai/backend/manager/services/app_config_fragment/actions/create.py b/src/ai/backend/manager/services/app_config_fragment/actions/create.py index b619a0ecb8e..ddcbff3101c 100644 --- a/src/ai/backend/manager/services/app_config_fragment/actions/create.py +++ b/src/ai/backend/manager/services/app_config_fragment/actions/create.py @@ -47,7 +47,8 @@ def target_element(self) -> RBACElementRef: element = self.creator_spec.scope_type.to_rbac_element_type() if element is None: return RBACElementRef(RBACElementType.APP_CONFIG_FRAGMENT, "") - return RBACElementRef(element, self.creator_spec.scope_id) + # A non-null element type means domain or user scope, which always carries an owner. + return RBACElementRef(element, str(self.creator_spec.scope_id)) @dataclass diff --git a/tests/unit/manager/repositories/app_config_fragment/test_repository.py b/tests/unit/manager/repositories/app_config_fragment/test_repository.py index de6d073f1d3..7e6ba8be22b 100644 --- a/tests/unit/manager/repositories/app_config_fragment/test_repository.py +++ b/tests/unit/manager/repositories/app_config_fragment/test_repository.py @@ -60,9 +60,8 @@ _DOMAIN_UUID = uuid.uuid4() _USER_UUID = uuid.uuid4() -_DOMAIN_ID = str(_DOMAIN_UUID) -_USER_ID = str(_USER_UUID) -_OTHER_USER_ID = str(uuid.uuid4()) +_OTHER_DOMAIN_UUID = uuid.uuid4() +_OTHER_USER_UUID = uuid.uuid4() @pytest.fixture @@ -129,7 +128,7 @@ async def domain_scoped_fragment(database: ExtendedAsyncSAEngine) -> AppConfigFr row = AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_UUID, config={"k": "v"}, ) db_sess.add(row) @@ -160,37 +159,37 @@ async def fragments_across_scopes(database: ExtendedAsyncSAEngine) -> list[AppCo AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.PUBLIC, - scope_id="public", + scope_id=None, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_UUID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id="other", + scope_id=_OTHER_DOMAIN_UUID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_OTHER_USER_ID, + scope_id=_OTHER_USER_UUID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="menu", scope_type=AppConfigScopeType.PUBLIC, - scope_id="public", + scope_id=None, config={"k": "v"}, ), ] @@ -207,7 +206,7 @@ async def test_create_then_get_by_id( AppConfigFragmentCreatorSpec( config_name="theme", scope_type=AppConfigScopeType.PUBLIC, - scope_id="public", + scope_id=None, config={"theme": "dark"}, ) ) @@ -229,7 +228,7 @@ async def test_create_rejected_when_not_allow_listed( AppConfigFragmentCreatorSpec( config_name="theme", scope_type=AppConfigScopeType.PUBLIC, - scope_id="public", + scope_id=None, config={"theme": "dark"}, ) ) @@ -336,10 +335,10 @@ async def test_filter_by_scope_id( result = await repository.admin_search( BatchQuerier( pagination=OffsetPagination(limit=10, offset=0), - conditions=[AppConfigFragmentConditions.by_scope_id_equals(_USER_ID)], + conditions=[AppConfigFragmentConditions.by_scope_id_equals(_USER_UUID)], ) ) - expected = {f.id for f in fragments_across_scopes if f.scope_id == _USER_ID} + expected = {f.id for f in fragments_across_scopes if f.scope_id == _USER_UUID} assert {item.id for item in result.items} == expected @@ -357,7 +356,7 @@ async def test_domain_scope_returns_only_that_domain( expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID + if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID } assert {item.id for item in result.items} == expected assert result.total_count == len(expected) @@ -374,7 +373,7 @@ async def test_user_scope_returns_only_that_user( expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID + if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID } assert {item.id for item in result.items} == expected @@ -393,8 +392,8 @@ async def test_scopes_or_combined_across_domain_and_user( expected = { f.id for f in fragments_across_scopes - if (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID) - or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID) + if (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID) + or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID) } assert {item.id for item in result.items} == expected @@ -432,13 +431,13 @@ async def two_fragments(database: ExtendedAsyncSAEngine) -> list[AppConfigFragme AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_UUID, config={"a": 1}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"b": 2}, ), ] @@ -547,13 +546,15 @@ async def test_domain_visibility_selects_only_that_domain( result = await repository.admin_search( BatchQuerier( pagination=OffsetPagination(limit=10, offset=0), - conditions=[AppConfigFragmentConditions.by_domain_visibility(_DOMAIN_ID)], + conditions=[ + AppConfigFragmentConditions.by_domain_visibility(DomainID(_DOMAIN_UUID)) + ], ) ) expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID + if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID } assert {item.id for item in result.items} == expected @@ -565,13 +566,13 @@ async def test_user_visibility_selects_only_that_user( result = await repository.admin_search( BatchQuerier( pagination=OffsetPagination(limit=10, offset=0), - conditions=[AppConfigFragmentConditions.by_user_visibility(_USER_ID)], + conditions=[AppConfigFragmentConditions.by_user_visibility(UserID(_USER_UUID))], ) ) expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID + if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID } assert {item.id for item in result.items} == expected @@ -594,8 +595,8 @@ async def test_one_query_returns_public_domain_user_rank_ordered( if f.config_name == "theme" and ( f.scope_type is AppConfigScopeType.PUBLIC - or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID) - or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID) + or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID) + or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID) ) ] assert [f.id for f in applicable] == [f.id for f in expected] @@ -632,8 +633,8 @@ async def test_bulk_returns_visible_fragments_for_all_names_ordered( if f.config_name in ("theme", "menu") and ( f.scope_type is AppConfigScopeType.PUBLIC - or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID) - or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID) + or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID) + or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID) ) } assert {f.id for f in applicable} == expected @@ -672,7 +673,7 @@ class _FragmentScopeCase: """ scope_type: AppConfigScopeType - scope_id: str + scope_id: uuid.UUID | None expected_bindings: list[_ScopeBinding] = field(default_factory=list) @@ -699,15 +700,19 @@ async def _scope_bindings( [ _FragmentScopeCase( scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, - expected_bindings=[_ScopeBinding(scope_type=ScopeType.USER, scope_id=_USER_ID)], + scope_id=_USER_UUID, + expected_bindings=[ + _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_UUID)) + ], ), _FragmentScopeCase( scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, - expected_bindings=[_ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=_DOMAIN_ID)], + scope_id=_DOMAIN_UUID, + expected_bindings=[ + _ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=str(_DOMAIN_UUID)) + ], ), - _FragmentScopeCase(scope_type=AppConfigScopeType.PUBLIC, scope_id="public"), + _FragmentScopeCase(scope_type=AppConfigScopeType.PUBLIC, scope_id=None), ], ids=lambda case: case.scope_type.value, ) @@ -733,15 +738,19 @@ async def test_create_binds_to_its_rbac_scope( [ _FragmentScopeCase( scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, - expected_bindings=[_ScopeBinding(scope_type=ScopeType.USER, scope_id=_USER_ID)], + scope_id=_USER_UUID, + expected_bindings=[ + _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_UUID)) + ], ), _FragmentScopeCase( scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, - expected_bindings=[_ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=_DOMAIN_ID)], + scope_id=_DOMAIN_UUID, + expected_bindings=[ + _ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=str(_DOMAIN_UUID)) + ], ), - _FragmentScopeCase(scope_type=AppConfigScopeType.PUBLIC, scope_id="public"), + _FragmentScopeCase(scope_type=AppConfigScopeType.PUBLIC, scope_id=None), ], ids=lambda case: case.scope_type.value, ) @@ -780,12 +789,12 @@ async def test_bulk_purge_removes_the_scope_binding( AppConfigFragmentCreatorSpec( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, ) ) assert await self._scope_bindings(database, str(created.id)) == [ - _ScopeBinding(scope_type=ScopeType.USER, scope_id=_USER_ID) + _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_UUID)) ] result = await repository.bulk_purge([AppConfigFragmentPurgerSpec(fragment_id=created.id)]) assert [p.id for p in result.succeeded] == [created.id] From 7ba7fdb826f545b3be663193dbc98683685b2c4c Mon Sep 17 00:00:00 2001 From: Gyubong Date: Tue, 21 Jul 2026 14:37:17 +0900 Subject: [PATCH 2/6] fix(BA-6948): enforce the scope_id/scope_type invariant and fix fallout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups: - Cast a domain/user scope_id directly instead of through NULLIF(scope_id, ''). The NULLIF turned an empty owner into NULL, which contradicts the fail-loud intent and would have left that fragment ownerless. - Add a check constraint tying the columns together, so the invariant the partial index assumes — NULL scope_id exactly when the scope is public — is enforced rather than documented. Without it a public row could carry an owner and slip past the partial index, or a domain row could have none. - Cover public in the duplicate-write test. It is the case the partial index carries, and the test only exercised a domain fragment before, so removing the index would have gone unnoticed. Parametrized over every scope type. The remaining test changes are fallout from the column type: several service and allow-list tests built fragments with string scope_ids. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../backend/common/data/app_config/types.py | 6 +- ...a8_app_config_fragment_scope_id_to_uuid.py | 38 ++++----- .../manager/models/app_config_fragment/row.py | 12 +-- .../app_config_allow_list/test_repository.py | 4 +- .../app_config_fragment/test_repository.py | 44 ++++++++-- .../services/app_config/test_service.py | 28 ++++--- .../app_config_fragment/test_create_action.py | 18 +++-- .../app_config_fragment/test_service.py | 80 +++++++++++++------ 8 files changed, 150 insertions(+), 80 deletions(-) diff --git a/src/ai/backend/common/data/app_config/types.py b/src/ai/backend/common/data/app_config/types.py index d296f0b7a41..77822a0bc9b 100644 --- a/src/ai/backend/common/data/app_config/types.py +++ b/src/ai/backend/common/data/app_config/types.py @@ -51,11 +51,9 @@ def to_rbac_element_type(self) -> RBACElementType | None: return RBACElementType.USER def to_rbac_scope_id(self, scope_id: uuid.UUID | None) -> str: - """The RBAC scope id for a write at this fragment scope. + """The RBAC scope id for a write at this fragment scope, in RBAC's string form. - ``public`` is system-wide (no per-entity scope id); ``domain`` / ``user`` carry - their own ``scope_id``. RBAC identifies scopes by string, so the owner id is - rendered as text here even though it is stored as a UUID. + ``public`` is system-wide and names no owner. """ return "" if self is AppConfigScopeType.PUBLIC else str(scope_id) diff --git a/src/ai/backend/manager/models/alembic/versions/e5b71c94d2a8_app_config_fragment_scope_id_to_uuid.py b/src/ai/backend/manager/models/alembic/versions/e5b71c94d2a8_app_config_fragment_scope_id_to_uuid.py index bc2c233f86f..196db4f9140 100644 --- a/src/ai/backend/manager/models/alembic/versions/e5b71c94d2a8_app_config_fragment_scope_id_to_uuid.py +++ b/src/ai/backend/manager/models/alembic/versions/e5b71c94d2a8_app_config_fragment_scope_id_to_uuid.py @@ -1,23 +1,16 @@ """convert app_config_fragments.scope_id to a nullable UUID -``scope_id`` held a ``VARCHAR(255)`` that was never really a string: a domain -fragment stores a domain id and a user fragment a user id, both UUIDs, while a -public fragment has no owner at all and stored ``''`` only because the column -was ``NOT NULL``. Store the real type instead — ``UUID NULL``, with ``NULL`` -meaning "public, no owner". +``scope_id`` was a ``VARCHAR`` holding a domain or user UUID, with ``''`` for +public only because the column was ``NOT NULL``. It becomes ``UUID NULL``, where +``NULL`` is public. -The uniqueness of ``(config_name, scope_type, scope_id)`` needs help: Postgres -treats ``NULL``s as distinct in a unique constraint, so the existing constraint -stops rejecting a second public fragment for the same config name once public -rows hold ``NULL``. A partial unique index over the ``NULL`` rows restores the -guarantee the ``''`` sentinel used to provide. (``UNIQUE NULLS NOT DISTINCT`` -would say the same thing in one constraint, but it needs Postgres 15+ and the -test fixture runs 13.) +Public rows are forced to ``NULL``; a domain or user id that is not a UUID aborts +the migration rather than being nulled, since that binding decides who can see +the fragment. -Public rows are forced to ``NULL`` regardless of what they stored, since public -has no owner by definition. Domain and user rows are cast, and a value that is -not a UUID fails the migration on purpose — nulling it would silently drop the -scope binding that decides who can see the fragment. +``NULL``s are distinct to a unique constraint, so public rows get a partial +unique index (``UNIQUE NULLS NOT DISTINCT`` needs Postgres 15+; the test fixture +runs 13), and a check constraint keeps ``NULL`` and public in step. Revision ID: e5b71c94d2a8 Revises: 577c7a215934 @@ -36,6 +29,8 @@ depends_on = None _PUBLIC_INDEX = "uq_app_config_fragments_public_config_name" +# Bare name — the naming convention prefixes it with ck__. +_SCOPE_ID_CHECK = "scope_id_matches_scope_type" _TABLE = "app_config_fragments" @@ -48,11 +43,16 @@ def upgrade() -> None: USING ( CASE WHEN scope_type = 'public' THEN NULL - ELSE NULLIF(scope_id, '')::uuid + ELSE scope_id::uuid END ) """) ) + op.create_check_constraint( + _SCOPE_ID_CHECK, + _TABLE, + "(scope_type = 'public') = (scope_id IS NULL)", + ) op.create_index( _PUBLIC_INDEX, _TABLE, @@ -64,8 +64,8 @@ def upgrade() -> None: def downgrade() -> None: op.drop_index(_PUBLIC_INDEX, table_name=_TABLE) - # Public rows go back to the empty sentinel the NOT NULL column used to require, which - # the plain constraint can compare like any other value. + op.drop_constraint(_SCOPE_ID_CHECK, _TABLE, type_="check") + # Public rows go back to the empty sentinel the NOT NULL column required. op.execute( sa.text(""" ALTER TABLE app_config_fragments diff --git a/src/ai/backend/manager/models/app_config_fragment/row.py b/src/ai/backend/manager/models/app_config_fragment/row.py index 352e491038b..f61ab2c9dc6 100644 --- a/src/ai/backend/manager/models/app_config_fragment/row.py +++ b/src/ai/backend/manager/models/app_config_fragment/row.py @@ -33,10 +33,7 @@ class AppConfigFragmentRow(LifecycleTimestampsMixin, Base): # type: ignore[misc "scope_id", name="uq_app_config_fragments_config_name_scope_type_scope_id", ), - # The constraint above only covers domain and user fragments: Postgres counts NULLs - # as distinct, so it would let a config take any number of public fragments. This - # partial index restores that guarantee for the NULL (public) rows. It replaces the - # NULLS NOT DISTINCT the constraint would otherwise want, which needs Postgres 15+. + # NULLs are distinct to a unique constraint, so public rows need their own index. sa.Index( "uq_app_config_fragments_public_config_name", "config_name", @@ -44,6 +41,10 @@ class AppConfigFragmentRow(LifecycleTimestampsMixin, Base): # type: ignore[misc unique=True, postgresql_where=sa.text("scope_id IS NULL"), ), + sa.CheckConstraint( + "(scope_type = 'public') = (scope_id IS NULL)", + name="scope_id_matches_scope_type", + ), sa.ForeignKeyConstraint( ["config_name", "scope_type"], ["app_config_allow_list.config_name", "app_config_allow_list.scope_type"], @@ -68,8 +69,7 @@ class AppConfigFragmentRow(LifecycleTimestampsMixin, Base): # type: ignore[misc StrEnumType(AppConfigScopeType), nullable=False, ) - # NULL is the public scope: it has no owner. Domain and user fragments carry the id of - # the domain or user that owns them. + # NULL is public, which has no owner; domain and user carry their owner's id. scope_id: Mapped[uuid.UUID | None] = mapped_column( "scope_id", GUID, diff --git a/tests/unit/manager/repositories/app_config_allow_list/test_repository.py b/tests/unit/manager/repositories/app_config_allow_list/test_repository.py index 3275f76e39f..82f8d9fddbe 100644 --- a/tests/unit/manager/repositories/app_config_allow_list/test_repository.py +++ b/tests/unit/manager/repositories/app_config_allow_list/test_repository.py @@ -252,7 +252,7 @@ async def test_purge_cascades_to_fragments( AppConfigFragmentRow( config_name=existing_entry.config_name, scope_type=existing_entry.scope_type, - scope_id="public", + scope_id=None, config={"k": "v"}, ) ) @@ -282,7 +282,7 @@ async def test_definition_purge_cascades_to_allow_list_and_fragments( AppConfigFragmentRow( config_name=entry.config_name, scope_type=entry.scope_type, - scope_id="public", + scope_id=None, config={"k": "v"}, ) ) diff --git a/tests/unit/manager/repositories/app_config_fragment/test_repository.py b/tests/unit/manager/repositories/app_config_fragment/test_repository.py index 7e6ba8be22b..d1d239afa38 100644 --- a/tests/unit/manager/repositories/app_config_fragment/test_repository.py +++ b/tests/unit/manager/repositories/app_config_fragment/test_repository.py @@ -136,6 +136,33 @@ async def domain_scoped_fragment(database: ExtendedAsyncSAEngine) -> AppConfigFr return row.to_data() +@pytest.fixture +async def fragment_at_every_scope( + database: ExtendedAsyncSAEngine, theme_registered: None +) -> dict[AppConfigScopeType, uuid.UUID | None]: + """Situation: ``theme`` already holds one fragment at each scope. + + Returns the owner each scope was written at, so a test can aim a duplicate at it. + """ + scope_ids: dict[AppConfigScopeType, uuid.UUID | None] = { + AppConfigScopeType.PUBLIC: None, + AppConfigScopeType.DOMAIN: _DOMAIN_UUID, + AppConfigScopeType.USER: _USER_UUID, + } + async with database.begin_session() as db_sess: + db_sess.add_all([ + AppConfigFragmentRow( + config_name="theme", + scope_type=scope_type, + scope_id=scope_id, + config={"k": "v"}, + ) + for scope_type, scope_id in scope_ids.items() + ]) + await db_sess.flush() + return scope_ids + + @pytest.fixture async def fragments_across_scopes(database: ExtendedAsyncSAEngine) -> list[AppConfigFragmentData]: """Situation: fragments across every scope_type, two domains, two users, two config_names. @@ -233,17 +260,24 @@ async def test_create_rejected_when_not_allow_listed( ) ) - async def test_unique_constraint_violation( + @pytest.mark.parametrize( + "scope_type", + [AppConfigScopeType.PUBLIC, AppConfigScopeType.DOMAIN, AppConfigScopeType.USER], + ids=lambda scope_type: scope_type.value, + ) + async def test_a_second_fragment_at_the_same_scope_is_rejected( self, repository: AppConfigFragmentRepository, - domain_scoped_fragment: AppConfigFragmentData, + fragment_at_every_scope: dict[AppConfigScopeType, uuid.UUID | None], + scope_type: AppConfigScopeType, ) -> None: + # public is carried by the partial index, domain and user by the unique constraint. with pytest.raises(UniqueConstraintViolationError): await repository.create( AppConfigFragmentCreatorSpec( - config_name=domain_scoped_fragment.config_name, - scope_type=domain_scoped_fragment.scope_type, - scope_id=domain_scoped_fragment.scope_id, + config_name="theme", + scope_type=scope_type, + scope_id=fragment_at_every_scope[scope_type], config={"k": "v"}, ) ) diff --git a/tests/unit/manager/services/app_config/test_service.py b/tests/unit/manager/services/app_config/test_service.py index 1f923ac8c9c..4079d73cd57 100644 --- a/tests/unit/manager/services/app_config/test_service.py +++ b/tests/unit/manager/services/app_config/test_service.py @@ -31,7 +31,9 @@ _NOW = datetime.now(UTC) _SCOPE_ARGS = AppConfigScopeArguments(domain_id=_DOMAIN_ID) -FragmentFactory = Callable[[str, dict[str, Any], AppConfigScopeType, str], AppConfigFragmentData] +FragmentFactory = Callable[ + [str, dict[str, Any], AppConfigScopeType, uuid.UUID | None], AppConfigFragmentData +] @pytest.fixture @@ -45,7 +47,7 @@ def _make( config_name: str, config: dict[str, Any], scope_type: AppConfigScopeType, - scope_id: str, + scope_id: uuid.UUID | None, ) -> AppConfigFragmentData: return AppConfigFragmentData( id=AppConfigFragmentID(uuid.uuid4()), @@ -77,8 +79,10 @@ def deep_merge_fragments( ) -> list[AppConfigFragmentData]: # Rank-ordered (low -> high), so the user fragment overrides on merge. fragments = [ - make_fragment("theme", {"theme": "light", "lang": "en"}, AppConfigScopeType.PUBLIC, ""), - make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.USER, str(_USER_ID)), + make_fragment( + "theme", {"theme": "light", "lang": "en"}, AppConfigScopeType.PUBLIC, None + ), + make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.USER, _USER_ID), ] mock_fragment_repository.list_visible_fragments_bulk = AsyncMock(return_value=fragments) return fragments @@ -95,13 +99,13 @@ def list_replace_fragments( "ui", {"nav": ["home", "about", "contact"], "theme": {"light": True}}, AppConfigScopeType.PUBLIC, - "", + None, ), make_fragment( "ui", {"nav": ["dashboard"], "theme": {"dark": True}}, AppConfigScopeType.USER, - str(_USER_ID), + _USER_ID, ), ] mock_fragment_repository.list_visible_fragments_bulk = AsyncMock(return_value=fragments) @@ -124,9 +128,11 @@ def two_name_fragments( ) -> list[AppConfigFragmentData]: # Visible fragments for both names, (config_name, rank)-ordered. fragments = [ - make_fragment("theme", {"theme": "light", "lang": "en"}, AppConfigScopeType.PUBLIC, ""), - make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.USER, str(_USER_ID)), - make_fragment("menu", {"items": ["a"]}, AppConfigScopeType.PUBLIC, ""), + make_fragment( + "theme", {"theme": "light", "lang": "en"}, AppConfigScopeType.PUBLIC, None + ), + make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.USER, _USER_ID), + make_fragment("menu", {"items": ["a"]}, AppConfigScopeType.PUBLIC, None), ] mock_fragment_repository.list_visible_fragments_bulk = AsyncMock(return_value=fragments) return fragments @@ -137,7 +143,7 @@ def duplicate_name_fragments( make_fragment: FragmentFactory, mock_fragment_repository: MagicMock, ) -> list[AppConfigFragmentData]: - fragments = [make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.PUBLIC, "")] + fragments = [make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.PUBLIC, None)] mock_fragment_repository.list_visible_fragments_bulk = AsyncMock(return_value=fragments) return fragments @@ -149,7 +155,7 @@ def public_only_fragments( ) -> list[AppConfigFragmentData]: fragments = [ make_fragment( - "theme", {"theme": "light", "lang": "en"}, AppConfigScopeType.PUBLIC, "public" + "theme", {"theme": "light", "lang": "en"}, AppConfigScopeType.PUBLIC, None ) ] mock_fragment_repository.list_visible_fragments_bulk = AsyncMock(return_value=fragments) diff --git a/tests/unit/manager/services/app_config_fragment/test_create_action.py b/tests/unit/manager/services/app_config_fragment/test_create_action.py index a2a796c737b..8adad382864 100644 --- a/tests/unit/manager/services/app_config_fragment/test_create_action.py +++ b/tests/unit/manager/services/app_config_fragment/test_create_action.py @@ -7,6 +7,7 @@ from __future__ import annotations +import uuid from dataclasses import dataclass import pytest @@ -21,11 +22,14 @@ CreateAppConfigFragmentAction, ) +_VICTIM_USER_ID = uuid.uuid4() +_DOMAIN_ID = uuid.uuid4() + def _make_action( *, scope_type: AppConfigScopeType, - scope_id: str, + scope_id: uuid.UUID | None, ) -> CreateAppConfigFragmentAction: return CreateAppConfigFragmentAction( creator_spec=AppConfigFragmentCreatorSpec( @@ -42,7 +46,7 @@ class _ScopeTarget: """A fragment scope, and the RBAC scope a create at it must authorize against.""" scope_type: AppConfigScopeType - scope_id: str + scope_id: uuid.UUID | None expected_element: RBACElementRef expected_scope_type: ScopeType @@ -55,19 +59,19 @@ class TestCreateTargetElement: [ _ScopeTarget( scope_type=AppConfigScopeType.USER, - scope_id="victim-user", - expected_element=RBACElementRef(RBACElementType.USER, "victim-user"), + scope_id=_VICTIM_USER_ID, + expected_element=RBACElementRef(RBACElementType.USER, str(_VICTIM_USER_ID)), expected_scope_type=ScopeType.USER, ), _ScopeTarget( scope_type=AppConfigScopeType.DOMAIN, - scope_id="default", - expected_element=RBACElementRef(RBACElementType.DOMAIN, "default"), + scope_id=_DOMAIN_ID, + expected_element=RBACElementRef(RBACElementType.DOMAIN, str(_DOMAIN_ID)), expected_scope_type=ScopeType.DOMAIN, ), _ScopeTarget( scope_type=AppConfigScopeType.PUBLIC, - scope_id="", + scope_id=None, expected_element=RBACElementRef(RBACElementType.APP_CONFIG_FRAGMENT, ""), expected_scope_type=ScopeType.GLOBAL, ), diff --git a/tests/unit/manager/services/app_config_fragment/test_service.py b/tests/unit/manager/services/app_config_fragment/test_service.py index c47a6eaa3c2..2690963d8ef 100644 --- a/tests/unit/manager/services/app_config_fragment/test_service.py +++ b/tests/unit/manager/services/app_config_fragment/test_service.py @@ -3,6 +3,7 @@ from __future__ import annotations import uuid +from dataclasses import dataclass from datetime import UTC, datetime from unittest.mock import AsyncMock, MagicMock @@ -66,10 +67,24 @@ from ai.backend.manager.types import OptionalState _USER_UUID = uuid.uuid4() -_USER_ID = str(_USER_UUID) +_DOMAIN_UUID = uuid.uuid4() _DOMAIN_ID = str(uuid.uuid4()) +@dataclass(frozen=True) +class _RBACScopeCase: + """A fragment scope, and the RBAC scope a create at it authorizes against. + + RBAC identifies scopes by string, so the expected id is the rendered form — empty for + public, which is global and names no owner. + """ + + scope_type: AppConfigScopeType + scope_id: uuid.UUID | None + expected_scope_type: ScopeType + expected_scope_id: str + + class TestAppConfigFragmentService: @pytest.fixture def mock_repository(self) -> MagicMock: @@ -90,7 +105,7 @@ async def test_create_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -99,7 +114,7 @@ async def test_create_delegates_to_repository( spec = AppConfigFragmentCreatorSpec( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, ) @@ -115,7 +130,7 @@ async def test_get(self, service: AppConfigFragmentService, mock_repository: Mag id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -145,7 +160,7 @@ async def test_admin_search( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -173,7 +188,7 @@ async def test_scoped_search_builds_domain_and_user_scopes( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -201,7 +216,10 @@ async def test_scoped_search_builds_domain_and_user_scopes( assert result.data == [fragment] # queried_refs preserve the scoped principals (domain, then user). - assert [ref.element_id for ref in result.queried_refs] == [str(domain_id), _USER_ID] + assert [ref.element_id for ref in result.queried_refs] == [ + str(domain_id), + str(_USER_UUID), + ] mock_repository.scoped_search.assert_called_once() called_querier, called_scopes = mock_repository.scoped_search.call_args.args assert called_querier is querier @@ -217,7 +235,7 @@ async def test_update_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -242,7 +260,7 @@ async def test_purge_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -265,7 +283,7 @@ async def test_bulk_update_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -296,7 +314,7 @@ async def test_bulk_purge_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_UUID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -324,27 +342,37 @@ class TestCreateActionScope: """The create action acts at the fragment's own scope — not admin-only/global.""" @pytest.mark.parametrize( - ("scope_type", "scope_id", "expected_scope_type", "expected_scope_id"), + "case", [ - (AppConfigScopeType.PUBLIC, "public", ScopeType.GLOBAL, ""), - (AppConfigScopeType.DOMAIN, "default", ScopeType.DOMAIN, "default"), - (AppConfigScopeType.USER, _USER_ID, ScopeType.USER, _USER_ID), + _RBACScopeCase( + scope_type=AppConfigScopeType.PUBLIC, + scope_id=None, + expected_scope_type=ScopeType.GLOBAL, + expected_scope_id="", + ), + _RBACScopeCase( + scope_type=AppConfigScopeType.DOMAIN, + scope_id=_DOMAIN_UUID, + expected_scope_type=ScopeType.DOMAIN, + expected_scope_id=str(_DOMAIN_UUID), + ), + _RBACScopeCase( + scope_type=AppConfigScopeType.USER, + scope_id=_USER_UUID, + expected_scope_type=ScopeType.USER, + expected_scope_id=str(_USER_UUID), + ), ], + ids=lambda case: case.scope_type.value, ) - def test_scope_follows_fragment_scope( - self, - scope_type: AppConfigScopeType, - scope_id: str, - expected_scope_type: ScopeType, - expected_scope_id: str, - ) -> None: + def test_scope_follows_fragment_scope(self, case: _RBACScopeCase) -> None: action = CreateAppConfigFragmentAction( creator_spec=AppConfigFragmentCreatorSpec( config_name="theme", - scope_type=scope_type, - scope_id=scope_id, + scope_type=case.scope_type, + scope_id=case.scope_id, config={"k": "v"}, ), ) - assert action.scope_type() == expected_scope_type - assert action.scope_id() == expected_scope_id + assert action.scope_type() == case.expected_scope_type + assert action.scope_id() == case.expected_scope_id From 9c62736f7414036128a0c53198e1546028c8d23e Mon Sep 17 00:00:00 2001 From: Gyubong Date: Tue, 21 Jul 2026 15:00:14 +0900 Subject: [PATCH 3/6] refactor(BA-6948): take a UUIDEqualMatchSpec in by_scope_id_equals The `_equals` conditions are what a filter adapter feeds from a UUIDFilter via convert_uuid_filter, which passes a UUIDEqualMatchSpec and expresses not_equals as negated=True. Taking a bare UUID could not be wired to equals_factory at all and left not_equals unrepresentable. Matches UserFairShareConditions.by_user_uuid. The by_*_visibility builders keep their DomainID / UserID: they are the internal visibility clause for one resolving principal, never user-supplied and never negated, so a spec would only add a state they cannot express. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../models/app_config_fragment/conditions.py | 9 ++++++--- .../app_config_fragment/test_repository.py | 17 +++++++++++++++-- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/ai/backend/manager/models/app_config_fragment/conditions.py b/src/ai/backend/manager/models/app_config_fragment/conditions.py index 37c55b60ab2..e622e5d6f77 100644 --- a/src/ai/backend/manager/models/app_config_fragment/conditions.py +++ b/src/ai/backend/manager/models/app_config_fragment/conditions.py @@ -9,7 +9,7 @@ import sqlalchemy as sa from ai.backend.common.data.app_config.types import AppConfigScopeType -from ai.backend.common.data.filter_specs import StringMatchSpec +from ai.backend.common.data.filter_specs import StringMatchSpec, UUIDEqualMatchSpec from ai.backend.common.identifier.domain import DomainID from ai.backend.common.identifier.user import UserID from ai.backend.manager.models.app_config_fragment.row import AppConfigFragmentRow @@ -120,9 +120,12 @@ def inner() -> sa.sql.expression.ColumnElement[bool]: # --- scope_id filter --- @staticmethod - def by_scope_id_equals(scope_id: uuid.UUID) -> QueryCondition: + def by_scope_id_equals(spec: UUIDEqualMatchSpec) -> QueryCondition: def inner() -> sa.sql.expression.ColumnElement[bool]: - return AppConfigFragmentRow.scope_id == scope_id + condition = AppConfigFragmentRow.scope_id == spec.value + if spec.negated: + condition = sa.not_(condition) + return condition return inner diff --git a/tests/unit/manager/repositories/app_config_fragment/test_repository.py b/tests/unit/manager/repositories/app_config_fragment/test_repository.py index d1d239afa38..d9ed08305a7 100644 --- a/tests/unit/manager/repositories/app_config_fragment/test_repository.py +++ b/tests/unit/manager/repositories/app_config_fragment/test_repository.py @@ -10,6 +10,7 @@ import sqlalchemy as sa from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.data.filter_specs import UUIDEqualMatchSpec from ai.backend.common.data.permission.types import EntityType, ScopeType from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID from ai.backend.common.identifier.domain import DomainID @@ -361,18 +362,30 @@ async def test_filter_by_scope_type( } assert {item.id for item in result.items} == expected + @pytest.mark.parametrize("negated", [False, True], ids=["equals", "not-equals"]) async def test_filter_by_scope_id( self, repository: AppConfigFragmentRepository, fragments_across_scopes: list[AppConfigFragmentData], + negated: bool, ) -> None: result = await repository.admin_search( BatchQuerier( pagination=OffsetPagination(limit=10, offset=0), - conditions=[AppConfigFragmentConditions.by_scope_id_equals(_USER_UUID)], + conditions=[ + AppConfigFragmentConditions.by_scope_id_equals( + UUIDEqualMatchSpec(value=_USER_UUID, negated=negated) + ) + ], ) ) - expected = {f.id for f in fragments_across_scopes if f.scope_id == _USER_UUID} + # Public rows hold NULL, and neither `= x` nor `NOT (= x)` is true of NULL, so they + # fall out of both directions of the filter. + expected = { + f.id + for f in fragments_across_scopes + if f.scope_id is not None and (f.scope_id == _USER_UUID) is not negated + } assert {item.id for item in result.items} == expected From 97c36a8bed7d2d50d3e7e194d482ec6165eb6aed Mon Sep 17 00:00:00 2001 From: Gyubong Date: Tue, 21 Jul 2026 16:31:56 +0900 Subject: [PATCH 4/6] refactor(BA-6948): name the scope owner type AppConfigScopeIdentifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bare `uuid.UUID | None` said nothing about which id a scope_id holds, and let any UUID through. `AppConfigScopeIdentifier = DomainID | UserID | None` states the pairing with AppConfigScopeType — a domain, a user, or nobody for public — and lives beside that enum as its companion. The union is not decoration: mypy rejected several call sites that were passing a raw uuid4() where a DomainID or UserID belongs, which the old annotation accepted silently. Test constants are now typed identifiers instead of bare UUIDs, and a dead `_DOMAIN_ID` string constant fell out. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../backend/common/data/app_config/types.py | 11 +- .../manager/data/app_config_fragment/types.py | 8 +- .../manager/models/app_config_fragment/row.py | 8 +- .../app_config_fragment/creators.py | 8 +- .../app_config_fragment/test_repository.py | 126 +++++++++--------- .../services/app_config/test_service.py | 9 +- .../app_config_fragment/test_create_action.py | 15 ++- .../app_config_fragment/test_service.py | 44 +++--- 8 files changed, 125 insertions(+), 104 deletions(-) diff --git a/src/ai/backend/common/data/app_config/types.py b/src/ai/backend/common/data/app_config/types.py index 77822a0bc9b..63b94585206 100644 --- a/src/ai/backend/common/data/app_config/types.py +++ b/src/ai/backend/common/data/app_config/types.py @@ -3,11 +3,16 @@ from __future__ import annotations import enum -import uuid from ai.backend.common.data.permission.types import RBACElementType, ScopeType +from ai.backend.common.identifier.domain import DomainID +from ai.backend.common.identifier.user import UserID -__all__ = ("AppConfigScopeType",) +__all__ = ("AppConfigScopeIdentifier", "AppConfigScopeType") + +# Who a fragment belongs to, paired with its ``AppConfigScopeType``: a domain, a user, or +# nobody for ``public``. +type AppConfigScopeIdentifier = DomainID | UserID | None class AppConfigScopeType(enum.StrEnum): @@ -50,7 +55,7 @@ def to_rbac_element_type(self) -> RBACElementType | None: case AppConfigScopeType.USER: return RBACElementType.USER - def to_rbac_scope_id(self, scope_id: uuid.UUID | None) -> str: + def to_rbac_scope_id(self, scope_id: AppConfigScopeIdentifier) -> str: """The RBAC scope id for a write at this fragment scope, in RBAC's string form. ``public`` is system-wide and names no owner. diff --git a/src/ai/backend/manager/data/app_config_fragment/types.py b/src/ai/backend/manager/data/app_config_fragment/types.py index 485f37c2266..6d99bf288ff 100644 --- a/src/ai/backend/manager/data/app_config_fragment/types.py +++ b/src/ai/backend/manager/data/app_config_fragment/types.py @@ -1,11 +1,13 @@ from __future__ import annotations -import uuid from dataclasses import dataclass from datetime import datetime from typing import Any -from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.data.app_config.types import ( + AppConfigScopeIdentifier, + AppConfigScopeType, +) from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID @@ -16,7 +18,7 @@ class AppConfigFragmentData: id: AppConfigFragmentID config_name: str scope_type: AppConfigScopeType - scope_id: uuid.UUID | None + scope_id: AppConfigScopeIdentifier config: dict[str, Any] created_at: datetime updated_at: datetime diff --git a/src/ai/backend/manager/models/app_config_fragment/row.py b/src/ai/backend/manager/models/app_config_fragment/row.py index f61ab2c9dc6..c834ff22fde 100644 --- a/src/ai/backend/manager/models/app_config_fragment/row.py +++ b/src/ai/backend/manager/models/app_config_fragment/row.py @@ -1,13 +1,15 @@ from __future__ import annotations -import uuid from typing import Any import sqlalchemy as sa from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.orm import Mapped, mapped_column -from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.data.app_config.types import ( + AppConfigScopeIdentifier, + AppConfigScopeType, +) from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID from ai.backend.manager.data.app_config_fragment.types import ( AppConfigFragmentData, @@ -70,7 +72,7 @@ class AppConfigFragmentRow(LifecycleTimestampsMixin, Base): # type: ignore[misc nullable=False, ) # NULL is public, which has no owner; domain and user carry their owner's id. - scope_id: Mapped[uuid.UUID | None] = mapped_column( + scope_id: Mapped[AppConfigScopeIdentifier] = mapped_column( "scope_id", GUID, nullable=True, diff --git a/src/ai/backend/manager/repositories/app_config_fragment/creators.py b/src/ai/backend/manager/repositories/app_config_fragment/creators.py index 43a0e53173a..8e6892ab63d 100644 --- a/src/ai/backend/manager/repositories/app_config_fragment/creators.py +++ b/src/ai/backend/manager/repositories/app_config_fragment/creators.py @@ -2,12 +2,14 @@ from __future__ import annotations -import uuid from collections.abc import Sequence from dataclasses import dataclass from typing import Any, override -from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.data.app_config.types import ( + AppConfigScopeIdentifier, + AppConfigScopeType, +) from ai.backend.manager.errors.app_config import AppConfigFragmentWriteNotAllowed from ai.backend.manager.errors.repository import ForeignKeyViolationError from ai.backend.manager.models.app_config_fragment.row import AppConfigFragmentRow @@ -24,7 +26,7 @@ class AppConfigFragmentCreatorSpec(CreatorSpec[AppConfigFragmentRow]): config_name: str scope_type: AppConfigScopeType - scope_id: uuid.UUID | None + scope_id: AppConfigScopeIdentifier config: dict[str, Any] @property diff --git a/tests/unit/manager/repositories/app_config_fragment/test_repository.py b/tests/unit/manager/repositories/app_config_fragment/test_repository.py index d9ed08305a7..f1240b5c571 100644 --- a/tests/unit/manager/repositories/app_config_fragment/test_repository.py +++ b/tests/unit/manager/repositories/app_config_fragment/test_repository.py @@ -9,7 +9,10 @@ import pytest import sqlalchemy as sa -from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.data.app_config.types import ( + AppConfigScopeIdentifier, + AppConfigScopeType, +) from ai.backend.common.data.filter_specs import UUIDEqualMatchSpec from ai.backend.common.data.permission.types import EntityType, ScopeType from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID @@ -59,10 +62,10 @@ from ai.backend.manager.types import OptionalState from ai.backend.testutils.db import with_tables -_DOMAIN_UUID = uuid.uuid4() -_USER_UUID = uuid.uuid4() -_OTHER_DOMAIN_UUID = uuid.uuid4() -_OTHER_USER_UUID = uuid.uuid4() +_DOMAIN_ID = DomainID(uuid.uuid4()) +_USER_ID = UserID(uuid.uuid4()) +_OTHER_DOMAIN_ID = DomainID(uuid.uuid4()) +_OTHER_USER_ID = UserID(uuid.uuid4()) @pytest.fixture @@ -129,7 +132,7 @@ async def domain_scoped_fragment(database: ExtendedAsyncSAEngine) -> AppConfigFr row = AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_UUID, + scope_id=_DOMAIN_ID, config={"k": "v"}, ) db_sess.add(row) @@ -140,28 +143,26 @@ async def domain_scoped_fragment(database: ExtendedAsyncSAEngine) -> AppConfigFr @pytest.fixture async def fragment_at_every_scope( database: ExtendedAsyncSAEngine, theme_registered: None -) -> dict[AppConfigScopeType, uuid.UUID | None]: - """Situation: ``theme`` already holds one fragment at each scope. - - Returns the owner each scope was written at, so a test can aim a duplicate at it. - """ - scope_ids: dict[AppConfigScopeType, uuid.UUID | None] = { +) -> dict[AppConfigScopeType, AppConfigFragmentData]: + """Situation: ``theme`` already holds one fragment at each scope, keyed by that scope.""" + owners: dict[AppConfigScopeType, AppConfigScopeIdentifier] = { AppConfigScopeType.PUBLIC: None, - AppConfigScopeType.DOMAIN: _DOMAIN_UUID, - AppConfigScopeType.USER: _USER_UUID, + AppConfigScopeType.DOMAIN: _DOMAIN_ID, + AppConfigScopeType.USER: _USER_ID, } async with database.begin_session() as db_sess: - db_sess.add_all([ + rows = [ AppConfigFragmentRow( config_name="theme", scope_type=scope_type, scope_id=scope_id, config={"k": "v"}, ) - for scope_type, scope_id in scope_ids.items() - ]) + for scope_type, scope_id in owners.items() + ] + db_sess.add_all(rows) await db_sess.flush() - return scope_ids + return {row.scope_type: row.to_data() for row in rows} @pytest.fixture @@ -193,25 +194,25 @@ async def fragments_across_scopes(database: ExtendedAsyncSAEngine) -> list[AppCo AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_UUID, + scope_id=_DOMAIN_ID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_OTHER_DOMAIN_UUID, + scope_id=_OTHER_DOMAIN_ID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_OTHER_USER_UUID, + scope_id=_OTHER_USER_ID, config={"k": "v"}, ), AppConfigFragmentRow( @@ -269,16 +270,17 @@ async def test_create_rejected_when_not_allow_listed( async def test_a_second_fragment_at_the_same_scope_is_rejected( self, repository: AppConfigFragmentRepository, - fragment_at_every_scope: dict[AppConfigScopeType, uuid.UUID | None], + fragment_at_every_scope: dict[AppConfigScopeType, AppConfigFragmentData], scope_type: AppConfigScopeType, ) -> None: # public is carried by the partial index, domain and user by the unique constraint. + existing = fragment_at_every_scope[scope_type] with pytest.raises(UniqueConstraintViolationError): await repository.create( AppConfigFragmentCreatorSpec( - config_name="theme", - scope_type=scope_type, - scope_id=fragment_at_every_scope[scope_type], + config_name=existing.config_name, + scope_type=existing.scope_type, + scope_id=existing.scope_id, config={"k": "v"}, ) ) @@ -374,7 +376,7 @@ async def test_filter_by_scope_id( pagination=OffsetPagination(limit=10, offset=0), conditions=[ AppConfigFragmentConditions.by_scope_id_equals( - UUIDEqualMatchSpec(value=_USER_UUID, negated=negated) + UUIDEqualMatchSpec(value=_USER_ID, negated=negated) ) ], ) @@ -384,7 +386,7 @@ async def test_filter_by_scope_id( expected = { f.id for f in fragments_across_scopes - if f.scope_id is not None and (f.scope_id == _USER_UUID) is not negated + if f.scope_id is not None and (f.scope_id == _USER_ID) is not negated } assert {item.id for item in result.items} == expected @@ -397,13 +399,13 @@ async def test_domain_scope_returns_only_that_domain( ) -> None: result = await repository.scoped_search( BatchQuerier(pagination=OffsetPagination(limit=10, offset=0)), - [DomainAppConfigFragmentSearchScope(domain_id=DomainID(_DOMAIN_UUID))], + [DomainAppConfigFragmentSearchScope(domain_id=_DOMAIN_ID)], ) # Only domain-scoped fragments of that domain — not the other domain, public, or users. expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID + if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID } assert {item.id for item in result.items} == expected assert result.total_count == len(expected) @@ -415,12 +417,12 @@ async def test_user_scope_returns_only_that_user( ) -> None: result = await repository.scoped_search( BatchQuerier(pagination=OffsetPagination(limit=10, offset=0)), - [UserAppConfigFragmentSearchScope(user_id=UserID(_USER_UUID))], + [UserAppConfigFragmentSearchScope(user_id=_USER_ID)], ) expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID + if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID } assert {item.id for item in result.items} == expected @@ -432,15 +434,15 @@ async def test_scopes_or_combined_across_domain_and_user( result = await repository.scoped_search( BatchQuerier(pagination=OffsetPagination(limit=10, offset=0)), [ - DomainAppConfigFragmentSearchScope(domain_id=DomainID(_DOMAIN_UUID)), - UserAppConfigFragmentSearchScope(user_id=UserID(_USER_UUID)), + DomainAppConfigFragmentSearchScope(domain_id=_DOMAIN_ID), + UserAppConfigFragmentSearchScope(user_id=_USER_ID), ], ) expected = { f.id for f in fragments_across_scopes - if (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID) - or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID) + if (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID) + or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID) } assert {item.id for item in result.items} == expected @@ -478,13 +480,13 @@ async def two_fragments(database: ExtendedAsyncSAEngine) -> list[AppConfigFragme AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_UUID, + scope_id=_DOMAIN_ID, config={"a": 1}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"b": 2}, ), ] @@ -593,15 +595,13 @@ async def test_domain_visibility_selects_only_that_domain( result = await repository.admin_search( BatchQuerier( pagination=OffsetPagination(limit=10, offset=0), - conditions=[ - AppConfigFragmentConditions.by_domain_visibility(DomainID(_DOMAIN_UUID)) - ], + conditions=[AppConfigFragmentConditions.by_domain_visibility(_DOMAIN_ID)], ) ) expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID + if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID } assert {item.id for item in result.items} == expected @@ -613,13 +613,13 @@ async def test_user_visibility_selects_only_that_user( result = await repository.admin_search( BatchQuerier( pagination=OffsetPagination(limit=10, offset=0), - conditions=[AppConfigFragmentConditions.by_user_visibility(UserID(_USER_UUID))], + conditions=[AppConfigFragmentConditions.by_user_visibility(_USER_ID)], ) ) expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID + if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID } assert {item.id for item in result.items} == expected @@ -632,7 +632,7 @@ async def test_one_query_returns_public_domain_user_rank_ordered( ) -> None: applicable = await repository.list_visible_fragments_bulk( ["theme"], - ResolvedAppConfigScope(domain_id=DomainID(_DOMAIN_UUID), user_id=UserID(_USER_UUID)), + ResolvedAppConfigScope(domain_id=_DOMAIN_ID, user_id=_USER_ID), ) # public + the caller's domain + the caller's own user fragment, ordered by the # allow-list entries' ranks (scope-type defaults: public < domain < user). @@ -642,8 +642,8 @@ async def test_one_query_returns_public_domain_user_rank_ordered( if f.config_name == "theme" and ( f.scope_type is AppConfigScopeType.PUBLIC - or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID) - or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID) + or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID) + or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID) ) ] assert [f.id for f in applicable] == [f.id for f in expected] @@ -660,7 +660,7 @@ async def test_unknown_config_name_returns_empty( ) -> None: applicable = await repository.list_visible_fragments_bulk( ["unregistered"], - ResolvedAppConfigScope(domain_id=DomainID(_DOMAIN_UUID), user_id=UserID(_USER_UUID)), + ResolvedAppConfigScope(domain_id=_DOMAIN_ID, user_id=_USER_ID), ) assert applicable == [] @@ -671,7 +671,7 @@ async def test_bulk_returns_visible_fragments_for_all_names_ordered( ) -> None: applicable = await repository.list_visible_fragments_bulk( ["theme", "menu"], - ResolvedAppConfigScope(domain_id=DomainID(_DOMAIN_UUID), user_id=UserID(_USER_UUID)), + ResolvedAppConfigScope(domain_id=_DOMAIN_ID, user_id=_USER_ID), ) # public + the caller's domain + the caller's own user fragment, for both names. expected = { @@ -680,8 +680,8 @@ async def test_bulk_returns_visible_fragments_for_all_names_ordered( if f.config_name in ("theme", "menu") and ( f.scope_type is AppConfigScopeType.PUBLIC - or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_UUID) - or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_UUID) + or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID) + or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID) ) } assert {f.id for f in applicable} == expected @@ -698,7 +698,7 @@ async def test_bulk_empty_names_returns_empty( ) -> None: applicable = await repository.list_visible_fragments_bulk( [], - ResolvedAppConfigScope(domain_id=DomainID(_DOMAIN_UUID), user_id=UserID(_USER_UUID)), + ResolvedAppConfigScope(domain_id=_DOMAIN_ID, user_id=_USER_ID), ) assert applicable == [] @@ -720,7 +720,7 @@ class _FragmentScopeCase: """ scope_type: AppConfigScopeType - scope_id: uuid.UUID | None + scope_id: AppConfigScopeIdentifier expected_bindings: list[_ScopeBinding] = field(default_factory=list) @@ -747,16 +747,16 @@ async def _scope_bindings( [ _FragmentScopeCase( scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, expected_bindings=[ - _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_UUID)) + _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_ID)) ], ), _FragmentScopeCase( scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_UUID, + scope_id=_DOMAIN_ID, expected_bindings=[ - _ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=str(_DOMAIN_UUID)) + _ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=str(_DOMAIN_ID)) ], ), _FragmentScopeCase(scope_type=AppConfigScopeType.PUBLIC, scope_id=None), @@ -785,16 +785,16 @@ async def test_create_binds_to_its_rbac_scope( [ _FragmentScopeCase( scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, expected_bindings=[ - _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_UUID)) + _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_ID)) ], ), _FragmentScopeCase( scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_UUID, + scope_id=_DOMAIN_ID, expected_bindings=[ - _ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=str(_DOMAIN_UUID)) + _ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=str(_DOMAIN_ID)) ], ), _FragmentScopeCase(scope_type=AppConfigScopeType.PUBLIC, scope_id=None), @@ -836,12 +836,12 @@ async def test_bulk_purge_removes_the_scope_binding( AppConfigFragmentCreatorSpec( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, ) ) assert await self._scope_bindings(database, str(created.id)) == [ - _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_UUID)) + _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_ID)) ] result = await repository.bulk_purge([AppConfigFragmentPurgerSpec(fragment_id=created.id)]) assert [p.id for p in result.succeeded] == [created.id] diff --git a/tests/unit/manager/services/app_config/test_service.py b/tests/unit/manager/services/app_config/test_service.py index 4079d73cd57..6d4f06c2336 100644 --- a/tests/unit/manager/services/app_config/test_service.py +++ b/tests/unit/manager/services/app_config/test_service.py @@ -10,7 +10,10 @@ import pytest -from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.data.app_config.types import ( + AppConfigScopeIdentifier, + AppConfigScopeType, +) from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID from ai.backend.common.identifier.domain import DomainID from ai.backend.common.identifier.user import UserID @@ -32,7 +35,7 @@ _SCOPE_ARGS = AppConfigScopeArguments(domain_id=_DOMAIN_ID) FragmentFactory = Callable[ - [str, dict[str, Any], AppConfigScopeType, uuid.UUID | None], AppConfigFragmentData + [str, dict[str, Any], AppConfigScopeType, AppConfigScopeIdentifier], AppConfigFragmentData ] @@ -47,7 +50,7 @@ def _make( config_name: str, config: dict[str, Any], scope_type: AppConfigScopeType, - scope_id: uuid.UUID | None, + scope_id: AppConfigScopeIdentifier, ) -> AppConfigFragmentData: return AppConfigFragmentData( id=AppConfigFragmentID(uuid.uuid4()), diff --git a/tests/unit/manager/services/app_config_fragment/test_create_action.py b/tests/unit/manager/services/app_config_fragment/test_create_action.py index 8adad382864..6136810bd2a 100644 --- a/tests/unit/manager/services/app_config_fragment/test_create_action.py +++ b/tests/unit/manager/services/app_config_fragment/test_create_action.py @@ -12,8 +12,13 @@ import pytest -from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.data.app_config.types import ( + AppConfigScopeIdentifier, + AppConfigScopeType, +) from ai.backend.common.data.permission.types import RBACElementType, ScopeType +from ai.backend.common.identifier.domain import DomainID +from ai.backend.common.identifier.user import UserID from ai.backend.manager.data.permission.types import RBACElementRef from ai.backend.manager.repositories.app_config_fragment.creators import ( AppConfigFragmentCreatorSpec, @@ -22,14 +27,14 @@ CreateAppConfigFragmentAction, ) -_VICTIM_USER_ID = uuid.uuid4() -_DOMAIN_ID = uuid.uuid4() +_VICTIM_USER_ID = UserID(uuid.uuid4()) +_DOMAIN_ID = DomainID(uuid.uuid4()) def _make_action( *, scope_type: AppConfigScopeType, - scope_id: uuid.UUID | None, + scope_id: AppConfigScopeIdentifier, ) -> CreateAppConfigFragmentAction: return CreateAppConfigFragmentAction( creator_spec=AppConfigFragmentCreatorSpec( @@ -46,7 +51,7 @@ class _ScopeTarget: """A fragment scope, and the RBAC scope a create at it must authorize against.""" scope_type: AppConfigScopeType - scope_id: uuid.UUID | None + scope_id: AppConfigScopeIdentifier expected_element: RBACElementRef expected_scope_type: ScopeType diff --git a/tests/unit/manager/services/app_config_fragment/test_service.py b/tests/unit/manager/services/app_config_fragment/test_service.py index 2690963d8ef..a6aecfa7d2d 100644 --- a/tests/unit/manager/services/app_config_fragment/test_service.py +++ b/tests/unit/manager/services/app_config_fragment/test_service.py @@ -9,7 +9,10 @@ import pytest -from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.data.app_config.types import ( + AppConfigScopeIdentifier, + AppConfigScopeType, +) from ai.backend.common.data.permission.types import ScopeType from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID from ai.backend.common.identifier.domain import DomainID @@ -66,9 +69,8 @@ from ai.backend.manager.services.app_config_fragment.service import AppConfigFragmentService from ai.backend.manager.types import OptionalState -_USER_UUID = uuid.uuid4() -_DOMAIN_UUID = uuid.uuid4() -_DOMAIN_ID = str(uuid.uuid4()) +_USER_ID = UserID(uuid.uuid4()) +_DOMAIN_ID = DomainID(uuid.uuid4()) @dataclass(frozen=True) @@ -80,7 +82,7 @@ class _RBACScopeCase: """ scope_type: AppConfigScopeType - scope_id: uuid.UUID | None + scope_id: AppConfigScopeIdentifier expected_scope_type: ScopeType expected_scope_id: str @@ -105,7 +107,7 @@ async def test_create_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -114,7 +116,7 @@ async def test_create_delegates_to_repository( spec = AppConfigFragmentCreatorSpec( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, ) @@ -130,7 +132,7 @@ async def test_get(self, service: AppConfigFragmentService, mock_repository: Mag id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -160,7 +162,7 @@ async def test_admin_search( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -188,7 +190,7 @@ async def test_scoped_search_builds_domain_and_user_scopes( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -208,7 +210,7 @@ async def test_scoped_search_builds_domain_and_user_scopes( ScopedSearchAppConfigFragmentAction( items=[ DomainAppConfigFragmentTarget(domain_id=domain_id), - UserAppConfigFragmentTarget(user_id=UserID(_USER_UUID)), + UserAppConfigFragmentTarget(user_id=_USER_ID), ], querier=querier, ) @@ -218,13 +220,13 @@ async def test_scoped_search_builds_domain_and_user_scopes( # queried_refs preserve the scoped principals (domain, then user). assert [ref.element_id for ref in result.queried_refs] == [ str(domain_id), - str(_USER_UUID), + str(_USER_ID), ] mock_repository.scoped_search.assert_called_once() called_querier, called_scopes = mock_repository.scoped_search.call_args.args assert called_querier is querier assert called_scopes[0].domain_id == domain_id - assert called_scopes[1].user_id == _USER_UUID + assert called_scopes[1].user_id == _USER_ID # --- update --- @@ -235,7 +237,7 @@ async def test_update_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -260,7 +262,7 @@ async def test_purge_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -283,7 +285,7 @@ async def test_bulk_update_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -314,7 +316,7 @@ async def test_bulk_purge_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -352,15 +354,15 @@ class TestCreateActionScope: ), _RBACScopeCase( scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_UUID, + scope_id=_DOMAIN_ID, expected_scope_type=ScopeType.DOMAIN, - expected_scope_id=str(_DOMAIN_UUID), + expected_scope_id=str(_DOMAIN_ID), ), _RBACScopeCase( scope_type=AppConfigScopeType.USER, - scope_id=_USER_UUID, + scope_id=_USER_ID, expected_scope_type=ScopeType.USER, - expected_scope_id=str(_USER_UUID), + expected_scope_id=str(_USER_ID), ), ], ids=lambda case: case.scope_type.value, From dd42bab190f72295a1781f989dfec6e68dcf708f Mon Sep 17 00:00:00 2001 From: Gyubong Date: Tue, 21 Jul 2026 16:52:20 +0900 Subject: [PATCH 5/6] refactor(BA-6948): move AppConfigScopeIdentifier to the identifier package It names an id, so it belongs with the other identifiers rather than beside the scope enum. common/identifier/scope.py already carries the same idea for RBAC. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ai/backend/common/data/app_config/types.py | 9 ++------- src/ai/backend/common/identifier/app_config.py | 9 +++++++++ src/ai/backend/manager/data/app_config_fragment/types.py | 6 ++---- src/ai/backend/manager/models/app_config_fragment/row.py | 6 ++---- .../manager/repositories/app_config_fragment/creators.py | 6 ++---- .../repositories/app_config_fragment/test_repository.py | 6 ++---- tests/unit/manager/services/app_config/test_service.py | 6 ++---- .../services/app_config_fragment/test_create_action.py | 6 ++---- .../manager/services/app_config_fragment/test_service.py | 6 ++---- 9 files changed, 25 insertions(+), 35 deletions(-) create mode 100644 src/ai/backend/common/identifier/app_config.py diff --git a/src/ai/backend/common/data/app_config/types.py b/src/ai/backend/common/data/app_config/types.py index 63b94585206..f96855a02c8 100644 --- a/src/ai/backend/common/data/app_config/types.py +++ b/src/ai/backend/common/data/app_config/types.py @@ -5,14 +5,9 @@ import enum from ai.backend.common.data.permission.types import RBACElementType, ScopeType -from ai.backend.common.identifier.domain import DomainID -from ai.backend.common.identifier.user import UserID +from ai.backend.common.identifier.app_config import AppConfigScopeIdentifier -__all__ = ("AppConfigScopeIdentifier", "AppConfigScopeType") - -# Who a fragment belongs to, paired with its ``AppConfigScopeType``: a domain, a user, or -# nobody for ``public``. -type AppConfigScopeIdentifier = DomainID | UserID | None +__all__ = ("AppConfigScopeType",) class AppConfigScopeType(enum.StrEnum): diff --git a/src/ai/backend/common/identifier/app_config.py b/src/ai/backend/common/identifier/app_config.py new file mode 100644 index 00000000000..16b4bd0c6cf --- /dev/null +++ b/src/ai/backend/common/identifier/app_config.py @@ -0,0 +1,9 @@ +from ai.backend.common.identifier.domain import DomainID +from ai.backend.common.identifier.user import UserID + +__all__ = ("AppConfigScopeIdentifier",) + + +# Who an app config fragment belongs to, paired with its ``AppConfigScopeType``: a domain, +# a user, or nobody for ``public``. +type AppConfigScopeIdentifier = DomainID | UserID | None diff --git a/src/ai/backend/manager/data/app_config_fragment/types.py b/src/ai/backend/manager/data/app_config_fragment/types.py index 6d99bf288ff..aed9eafc0b2 100644 --- a/src/ai/backend/manager/data/app_config_fragment/types.py +++ b/src/ai/backend/manager/data/app_config_fragment/types.py @@ -4,10 +4,8 @@ from datetime import datetime from typing import Any -from ai.backend.common.data.app_config.types import ( - AppConfigScopeIdentifier, - AppConfigScopeType, -) +from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.identifier.app_config import AppConfigScopeIdentifier from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID diff --git a/src/ai/backend/manager/models/app_config_fragment/row.py b/src/ai/backend/manager/models/app_config_fragment/row.py index c834ff22fde..a79c75286e8 100644 --- a/src/ai/backend/manager/models/app_config_fragment/row.py +++ b/src/ai/backend/manager/models/app_config_fragment/row.py @@ -6,10 +6,8 @@ from sqlalchemy.dialects.postgresql import JSONB from sqlalchemy.orm import Mapped, mapped_column -from ai.backend.common.data.app_config.types import ( - AppConfigScopeIdentifier, - AppConfigScopeType, -) +from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.identifier.app_config import AppConfigScopeIdentifier from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID from ai.backend.manager.data.app_config_fragment.types import ( AppConfigFragmentData, diff --git a/src/ai/backend/manager/repositories/app_config_fragment/creators.py b/src/ai/backend/manager/repositories/app_config_fragment/creators.py index 8e6892ab63d..0162bc74cf2 100644 --- a/src/ai/backend/manager/repositories/app_config_fragment/creators.py +++ b/src/ai/backend/manager/repositories/app_config_fragment/creators.py @@ -6,10 +6,8 @@ from dataclasses import dataclass from typing import Any, override -from ai.backend.common.data.app_config.types import ( - AppConfigScopeIdentifier, - AppConfigScopeType, -) +from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.identifier.app_config import AppConfigScopeIdentifier from ai.backend.manager.errors.app_config import AppConfigFragmentWriteNotAllowed from ai.backend.manager.errors.repository import ForeignKeyViolationError from ai.backend.manager.models.app_config_fragment.row import AppConfigFragmentRow diff --git a/tests/unit/manager/repositories/app_config_fragment/test_repository.py b/tests/unit/manager/repositories/app_config_fragment/test_repository.py index f1240b5c571..35570469f40 100644 --- a/tests/unit/manager/repositories/app_config_fragment/test_repository.py +++ b/tests/unit/manager/repositories/app_config_fragment/test_repository.py @@ -9,12 +9,10 @@ import pytest import sqlalchemy as sa -from ai.backend.common.data.app_config.types import ( - AppConfigScopeIdentifier, - AppConfigScopeType, -) +from ai.backend.common.data.app_config.types import AppConfigScopeType from ai.backend.common.data.filter_specs import UUIDEqualMatchSpec from ai.backend.common.data.permission.types import EntityType, ScopeType +from ai.backend.common.identifier.app_config import AppConfigScopeIdentifier from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID from ai.backend.common.identifier.domain import DomainID from ai.backend.common.identifier.user import UserID diff --git a/tests/unit/manager/services/app_config/test_service.py b/tests/unit/manager/services/app_config/test_service.py index 6d4f06c2336..98e95dffc30 100644 --- a/tests/unit/manager/services/app_config/test_service.py +++ b/tests/unit/manager/services/app_config/test_service.py @@ -10,10 +10,8 @@ import pytest -from ai.backend.common.data.app_config.types import ( - AppConfigScopeIdentifier, - AppConfigScopeType, -) +from ai.backend.common.data.app_config.types import AppConfigScopeType +from ai.backend.common.identifier.app_config import AppConfigScopeIdentifier from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID from ai.backend.common.identifier.domain import DomainID from ai.backend.common.identifier.user import UserID diff --git a/tests/unit/manager/services/app_config_fragment/test_create_action.py b/tests/unit/manager/services/app_config_fragment/test_create_action.py index 6136810bd2a..c419a368cf2 100644 --- a/tests/unit/manager/services/app_config_fragment/test_create_action.py +++ b/tests/unit/manager/services/app_config_fragment/test_create_action.py @@ -12,11 +12,9 @@ import pytest -from ai.backend.common.data.app_config.types import ( - AppConfigScopeIdentifier, - AppConfigScopeType, -) +from ai.backend.common.data.app_config.types import AppConfigScopeType from ai.backend.common.data.permission.types import RBACElementType, ScopeType +from ai.backend.common.identifier.app_config import AppConfigScopeIdentifier from ai.backend.common.identifier.domain import DomainID from ai.backend.common.identifier.user import UserID from ai.backend.manager.data.permission.types import RBACElementRef diff --git a/tests/unit/manager/services/app_config_fragment/test_service.py b/tests/unit/manager/services/app_config_fragment/test_service.py index a6aecfa7d2d..959c6b6ab0d 100644 --- a/tests/unit/manager/services/app_config_fragment/test_service.py +++ b/tests/unit/manager/services/app_config_fragment/test_service.py @@ -9,11 +9,9 @@ import pytest -from ai.backend.common.data.app_config.types import ( - AppConfigScopeIdentifier, - AppConfigScopeType, -) +from ai.backend.common.data.app_config.types import AppConfigScopeType from ai.backend.common.data.permission.types import ScopeType +from ai.backend.common.identifier.app_config import AppConfigScopeIdentifier from ai.backend.common.identifier.app_config_fragment import AppConfigFragmentID from ai.backend.common.identifier.domain import DomainID from ai.backend.common.identifier.user import UserID From 19ed7e6c2945b13846d0732d6593ae85b1a7d12f Mon Sep 17 00:00:00 2001 From: Gyubong Date: Tue, 21 Jul 2026 18:07:12 +0900 Subject: [PATCH 6/6] refactor(BA-6948): keep None out of AppConfigScopeIdentifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The alias names what kind of id an owner is — a domain or a user. Whether an owner is present at all is a separate question, answered per use, so ``| None`` is spelled where it applies instead of being folded into the name. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../backend/common/data/app_config/types.py | 2 +- .../backend/common/identifier/app_config.py | 11 ++-- .../manager/data/app_config_fragment/types.py | 2 +- .../manager/models/app_config_fragment/row.py | 4 +- .../app_config_fragment/creators.py | 2 +- .../app_config_fragment/test_repository.py | 62 ++++++++++--------- .../services/app_config/test_service.py | 15 +++-- .../app_config_fragment/test_create_action.py | 10 +-- .../app_config_fragment/test_service.py | 28 +++++---- 9 files changed, 77 insertions(+), 59 deletions(-) diff --git a/src/ai/backend/common/data/app_config/types.py b/src/ai/backend/common/data/app_config/types.py index f96855a02c8..9a30ecb3eb0 100644 --- a/src/ai/backend/common/data/app_config/types.py +++ b/src/ai/backend/common/data/app_config/types.py @@ -50,7 +50,7 @@ def to_rbac_element_type(self) -> RBACElementType | None: case AppConfigScopeType.USER: return RBACElementType.USER - def to_rbac_scope_id(self, scope_id: AppConfigScopeIdentifier) -> str: + def to_rbac_scope_id(self, scope_id: AppConfigScopeIdentifier | None) -> str: """The RBAC scope id for a write at this fragment scope, in RBAC's string form. ``public`` is system-wide and names no owner. diff --git a/src/ai/backend/common/identifier/app_config.py b/src/ai/backend/common/identifier/app_config.py index 16b4bd0c6cf..76855dbc9f8 100644 --- a/src/ai/backend/common/identifier/app_config.py +++ b/src/ai/backend/common/identifier/app_config.py @@ -1,9 +1,10 @@ -from ai.backend.common.identifier.domain import DomainID -from ai.backend.common.identifier.user import UserID +from typing import NewType +from uuid import UUID __all__ = ("AppConfigScopeIdentifier",) -# Who an app config fragment belongs to, paired with its ``AppConfigScopeType``: a domain, -# a user, or nobody for ``public``. -type AppConfigScopeIdentifier = DomainID | UserID | None +# Who an app config fragment belongs to. Polymorphic across scope kinds (domain/user); the +# concrete kind is discriminated by the accompanying ``AppConfigScopeType``, and ``public`` +# has no owner at all, so its absence is spelled ``| None`` at each use. +AppConfigScopeIdentifier = NewType("AppConfigScopeIdentifier", UUID) diff --git a/src/ai/backend/manager/data/app_config_fragment/types.py b/src/ai/backend/manager/data/app_config_fragment/types.py index aed9eafc0b2..dfc79f78b79 100644 --- a/src/ai/backend/manager/data/app_config_fragment/types.py +++ b/src/ai/backend/manager/data/app_config_fragment/types.py @@ -16,7 +16,7 @@ class AppConfigFragmentData: id: AppConfigFragmentID config_name: str scope_type: AppConfigScopeType - scope_id: AppConfigScopeIdentifier + scope_id: AppConfigScopeIdentifier | None config: dict[str, Any] created_at: datetime updated_at: datetime diff --git a/src/ai/backend/manager/models/app_config_fragment/row.py b/src/ai/backend/manager/models/app_config_fragment/row.py index a79c75286e8..3205c018ebb 100644 --- a/src/ai/backend/manager/models/app_config_fragment/row.py +++ b/src/ai/backend/manager/models/app_config_fragment/row.py @@ -70,9 +70,9 @@ class AppConfigFragmentRow(LifecycleTimestampsMixin, Base): # type: ignore[misc nullable=False, ) # NULL is public, which has no owner; domain and user carry their owner's id. - scope_id: Mapped[AppConfigScopeIdentifier] = mapped_column( + scope_id: Mapped[AppConfigScopeIdentifier | None] = mapped_column( "scope_id", - GUID, + GUID(AppConfigScopeIdentifier), nullable=True, ) config: Mapped[dict[str, Any]] = mapped_column( diff --git a/src/ai/backend/manager/repositories/app_config_fragment/creators.py b/src/ai/backend/manager/repositories/app_config_fragment/creators.py index 0162bc74cf2..d7f5c503527 100644 --- a/src/ai/backend/manager/repositories/app_config_fragment/creators.py +++ b/src/ai/backend/manager/repositories/app_config_fragment/creators.py @@ -24,7 +24,7 @@ class AppConfigFragmentCreatorSpec(CreatorSpec[AppConfigFragmentRow]): config_name: str scope_type: AppConfigScopeType - scope_id: AppConfigScopeIdentifier + scope_id: AppConfigScopeIdentifier | None config: dict[str, Any] @property diff --git a/tests/unit/manager/repositories/app_config_fragment/test_repository.py b/tests/unit/manager/repositories/app_config_fragment/test_repository.py index 35570469f40..493f9925b36 100644 --- a/tests/unit/manager/repositories/app_config_fragment/test_repository.py +++ b/tests/unit/manager/repositories/app_config_fragment/test_repository.py @@ -65,6 +65,12 @@ _OTHER_DOMAIN_ID = DomainID(uuid.uuid4()) _OTHER_USER_ID = UserID(uuid.uuid4()) +# The same owners seen as a fragment's scope_id, which is polymorphic over scope kinds. +_DOMAIN_SCOPE_ID = AppConfigScopeIdentifier(_DOMAIN_ID) +_USER_SCOPE_ID = AppConfigScopeIdentifier(_USER_ID) +_OTHER_DOMAIN_SCOPE_ID = AppConfigScopeIdentifier(_OTHER_DOMAIN_ID) +_OTHER_USER_SCOPE_ID = AppConfigScopeIdentifier(_OTHER_USER_ID) + @pytest.fixture async def database( @@ -130,7 +136,7 @@ async def domain_scoped_fragment(database: ExtendedAsyncSAEngine) -> AppConfigFr row = AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_SCOPE_ID, config={"k": "v"}, ) db_sess.add(row) @@ -143,10 +149,10 @@ async def fragment_at_every_scope( database: ExtendedAsyncSAEngine, theme_registered: None ) -> dict[AppConfigScopeType, AppConfigFragmentData]: """Situation: ``theme`` already holds one fragment at each scope, keyed by that scope.""" - owners: dict[AppConfigScopeType, AppConfigScopeIdentifier] = { + owners: dict[AppConfigScopeType, AppConfigScopeIdentifier | None] = { AppConfigScopeType.PUBLIC: None, - AppConfigScopeType.DOMAIN: _DOMAIN_ID, - AppConfigScopeType.USER: _USER_ID, + AppConfigScopeType.DOMAIN: _DOMAIN_SCOPE_ID, + AppConfigScopeType.USER: _USER_SCOPE_ID, } async with database.begin_session() as db_sess: rows = [ @@ -192,25 +198,25 @@ async def fragments_across_scopes(database: ExtendedAsyncSAEngine) -> list[AppCo AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_SCOPE_ID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_OTHER_DOMAIN_ID, + scope_id=_OTHER_DOMAIN_SCOPE_ID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_OTHER_USER_ID, + scope_id=_OTHER_USER_SCOPE_ID, config={"k": "v"}, ), AppConfigFragmentRow( @@ -374,7 +380,7 @@ async def test_filter_by_scope_id( pagination=OffsetPagination(limit=10, offset=0), conditions=[ AppConfigFragmentConditions.by_scope_id_equals( - UUIDEqualMatchSpec(value=_USER_ID, negated=negated) + UUIDEqualMatchSpec(value=_USER_SCOPE_ID, negated=negated) ) ], ) @@ -384,7 +390,7 @@ async def test_filter_by_scope_id( expected = { f.id for f in fragments_across_scopes - if f.scope_id is not None and (f.scope_id == _USER_ID) is not negated + if f.scope_id is not None and (f.scope_id == _USER_SCOPE_ID) is not negated } assert {item.id for item in result.items} == expected @@ -403,7 +409,7 @@ async def test_domain_scope_returns_only_that_domain( expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID + if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_SCOPE_ID } assert {item.id for item in result.items} == expected assert result.total_count == len(expected) @@ -420,7 +426,7 @@ async def test_user_scope_returns_only_that_user( expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID + if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_SCOPE_ID } assert {item.id for item in result.items} == expected @@ -439,8 +445,8 @@ async def test_scopes_or_combined_across_domain_and_user( expected = { f.id for f in fragments_across_scopes - if (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID) - or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID) + if (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_SCOPE_ID) + or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_SCOPE_ID) } assert {item.id for item in result.items} == expected @@ -478,13 +484,13 @@ async def two_fragments(database: ExtendedAsyncSAEngine) -> list[AppConfigFragme AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_SCOPE_ID, config={"a": 1}, ), AppConfigFragmentRow( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"b": 2}, ), ] @@ -599,7 +605,7 @@ async def test_domain_visibility_selects_only_that_domain( expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID + if f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_SCOPE_ID } assert {item.id for item in result.items} == expected @@ -617,7 +623,7 @@ async def test_user_visibility_selects_only_that_user( expected = { f.id for f in fragments_across_scopes - if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID + if f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_SCOPE_ID } assert {item.id for item in result.items} == expected @@ -640,8 +646,8 @@ async def test_one_query_returns_public_domain_user_rank_ordered( if f.config_name == "theme" and ( f.scope_type is AppConfigScopeType.PUBLIC - or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID) - or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID) + or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_SCOPE_ID) + or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_SCOPE_ID) ) ] assert [f.id for f in applicable] == [f.id for f in expected] @@ -678,8 +684,8 @@ async def test_bulk_returns_visible_fragments_for_all_names_ordered( if f.config_name in ("theme", "menu") and ( f.scope_type is AppConfigScopeType.PUBLIC - or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_ID) - or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_ID) + or (f.scope_type is AppConfigScopeType.DOMAIN and f.scope_id == _DOMAIN_SCOPE_ID) + or (f.scope_type is AppConfigScopeType.USER and f.scope_id == _USER_SCOPE_ID) ) } assert {f.id for f in applicable} == expected @@ -718,7 +724,7 @@ class _FragmentScopeCase: """ scope_type: AppConfigScopeType - scope_id: AppConfigScopeIdentifier + scope_id: AppConfigScopeIdentifier | None expected_bindings: list[_ScopeBinding] = field(default_factory=list) @@ -745,14 +751,14 @@ async def _scope_bindings( [ _FragmentScopeCase( scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, expected_bindings=[ _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_ID)) ], ), _FragmentScopeCase( scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_SCOPE_ID, expected_bindings=[ _ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=str(_DOMAIN_ID)) ], @@ -783,14 +789,14 @@ async def test_create_binds_to_its_rbac_scope( [ _FragmentScopeCase( scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, expected_bindings=[ _ScopeBinding(scope_type=ScopeType.USER, scope_id=str(_USER_ID)) ], ), _FragmentScopeCase( scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_SCOPE_ID, expected_bindings=[ _ScopeBinding(scope_type=ScopeType.DOMAIN, scope_id=str(_DOMAIN_ID)) ], @@ -834,7 +840,7 @@ async def test_bulk_purge_removes_the_scope_binding( AppConfigFragmentCreatorSpec( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, ) ) diff --git a/tests/unit/manager/services/app_config/test_service.py b/tests/unit/manager/services/app_config/test_service.py index 98e95dffc30..ffc5ab065fa 100644 --- a/tests/unit/manager/services/app_config/test_service.py +++ b/tests/unit/manager/services/app_config/test_service.py @@ -29,11 +29,16 @@ _USER_ID = UserID(uuid.uuid4()) _DOMAIN_ID = DomainID(uuid.uuid4()) + +# The same owners seen as a fragment's scope_id, which is polymorphic over scope kinds. +_USER_SCOPE_ID = AppConfigScopeIdentifier(_USER_ID) +_DOMAIN_SCOPE_ID = AppConfigScopeIdentifier(_DOMAIN_ID) _NOW = datetime.now(UTC) _SCOPE_ARGS = AppConfigScopeArguments(domain_id=_DOMAIN_ID) FragmentFactory = Callable[ - [str, dict[str, Any], AppConfigScopeType, AppConfigScopeIdentifier], AppConfigFragmentData + [str, dict[str, Any], AppConfigScopeType, AppConfigScopeIdentifier | None], + AppConfigFragmentData, ] @@ -48,7 +53,7 @@ def _make( config_name: str, config: dict[str, Any], scope_type: AppConfigScopeType, - scope_id: AppConfigScopeIdentifier, + scope_id: AppConfigScopeIdentifier | None, ) -> AppConfigFragmentData: return AppConfigFragmentData( id=AppConfigFragmentID(uuid.uuid4()), @@ -83,7 +88,7 @@ def deep_merge_fragments( make_fragment( "theme", {"theme": "light", "lang": "en"}, AppConfigScopeType.PUBLIC, None ), - make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.USER, _USER_ID), + make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.USER, _USER_SCOPE_ID), ] mock_fragment_repository.list_visible_fragments_bulk = AsyncMock(return_value=fragments) return fragments @@ -106,7 +111,7 @@ def list_replace_fragments( "ui", {"nav": ["dashboard"], "theme": {"dark": True}}, AppConfigScopeType.USER, - _USER_ID, + _USER_SCOPE_ID, ), ] mock_fragment_repository.list_visible_fragments_bulk = AsyncMock(return_value=fragments) @@ -132,7 +137,7 @@ def two_name_fragments( make_fragment( "theme", {"theme": "light", "lang": "en"}, AppConfigScopeType.PUBLIC, None ), - make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.USER, _USER_ID), + make_fragment("theme", {"theme": "dark"}, AppConfigScopeType.USER, _USER_SCOPE_ID), make_fragment("menu", {"items": ["a"]}, AppConfigScopeType.PUBLIC, None), ] mock_fragment_repository.list_visible_fragments_bulk = AsyncMock(return_value=fragments) diff --git a/tests/unit/manager/services/app_config_fragment/test_create_action.py b/tests/unit/manager/services/app_config_fragment/test_create_action.py index c419a368cf2..8a9e82b35fb 100644 --- a/tests/unit/manager/services/app_config_fragment/test_create_action.py +++ b/tests/unit/manager/services/app_config_fragment/test_create_action.py @@ -27,12 +27,14 @@ _VICTIM_USER_ID = UserID(uuid.uuid4()) _DOMAIN_ID = DomainID(uuid.uuid4()) +_VICTIM_USER_SCOPE_ID = AppConfigScopeIdentifier(_VICTIM_USER_ID) +_DOMAIN_SCOPE_ID = AppConfigScopeIdentifier(_DOMAIN_ID) def _make_action( *, scope_type: AppConfigScopeType, - scope_id: AppConfigScopeIdentifier, + scope_id: AppConfigScopeIdentifier | None, ) -> CreateAppConfigFragmentAction: return CreateAppConfigFragmentAction( creator_spec=AppConfigFragmentCreatorSpec( @@ -49,7 +51,7 @@ class _ScopeTarget: """A fragment scope, and the RBAC scope a create at it must authorize against.""" scope_type: AppConfigScopeType - scope_id: AppConfigScopeIdentifier + scope_id: AppConfigScopeIdentifier | None expected_element: RBACElementRef expected_scope_type: ScopeType @@ -62,13 +64,13 @@ class TestCreateTargetElement: [ _ScopeTarget( scope_type=AppConfigScopeType.USER, - scope_id=_VICTIM_USER_ID, + scope_id=_VICTIM_USER_SCOPE_ID, expected_element=RBACElementRef(RBACElementType.USER, str(_VICTIM_USER_ID)), expected_scope_type=ScopeType.USER, ), _ScopeTarget( scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_SCOPE_ID, expected_element=RBACElementRef(RBACElementType.DOMAIN, str(_DOMAIN_ID)), expected_scope_type=ScopeType.DOMAIN, ), diff --git a/tests/unit/manager/services/app_config_fragment/test_service.py b/tests/unit/manager/services/app_config_fragment/test_service.py index 959c6b6ab0d..6f91383a17f 100644 --- a/tests/unit/manager/services/app_config_fragment/test_service.py +++ b/tests/unit/manager/services/app_config_fragment/test_service.py @@ -70,6 +70,10 @@ _USER_ID = UserID(uuid.uuid4()) _DOMAIN_ID = DomainID(uuid.uuid4()) +# The same owners seen as a fragment's scope_id, which is polymorphic over scope kinds. +_USER_SCOPE_ID = AppConfigScopeIdentifier(_USER_ID) +_DOMAIN_SCOPE_ID = AppConfigScopeIdentifier(_DOMAIN_ID) + @dataclass(frozen=True) class _RBACScopeCase: @@ -80,7 +84,7 @@ class _RBACScopeCase: """ scope_type: AppConfigScopeType - scope_id: AppConfigScopeIdentifier + scope_id: AppConfigScopeIdentifier | None expected_scope_type: ScopeType expected_scope_id: str @@ -105,7 +109,7 @@ async def test_create_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -114,7 +118,7 @@ async def test_create_delegates_to_repository( spec = AppConfigFragmentCreatorSpec( config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, ) @@ -130,7 +134,7 @@ async def test_get(self, service: AppConfigFragmentService, mock_repository: Mag id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -160,7 +164,7 @@ async def test_admin_search( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -188,7 +192,7 @@ async def test_scoped_search_builds_domain_and_user_scopes( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -235,7 +239,7 @@ async def test_update_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -260,7 +264,7 @@ async def test_purge_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -283,7 +287,7 @@ async def test_bulk_update_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -314,7 +318,7 @@ async def test_bulk_purge_delegates_to_repository( id=AppConfigFragmentID(uuid.uuid4()), config_name="theme", scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, config={"k": "v"}, created_at=datetime.now(UTC), updated_at=datetime.now(UTC), @@ -352,13 +356,13 @@ class TestCreateActionScope: ), _RBACScopeCase( scope_type=AppConfigScopeType.DOMAIN, - scope_id=_DOMAIN_ID, + scope_id=_DOMAIN_SCOPE_ID, expected_scope_type=ScopeType.DOMAIN, expected_scope_id=str(_DOMAIN_ID), ), _RBACScopeCase( scope_type=AppConfigScopeType.USER, - scope_id=_USER_ID, + scope_id=_USER_SCOPE_ID, expected_scope_type=ScopeType.USER, expected_scope_id=str(_USER_ID), ),