Skip to content

Commit 7c50d93

Browse files
jopemachineclaude
andcommitted
feat(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) <noreply@anthropic.com>
1 parent 66fde7b commit 7c50d93

11 files changed

Lines changed: 169 additions & 60 deletions

File tree

changes/12984.feature.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Store `app_config_fragments.scope_id` as a nullable UUID, with `NULL` for the ownerless public scope, and accept and return it as a UUID on the REST v2 API.

src/ai/backend/common/data/app_config/types.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
from __future__ import annotations
44

55
import enum
6+
import uuid
67

78
from ai.backend.common.data.permission.types import RBACElementType, ScopeType
89

@@ -49,13 +50,14 @@ def to_rbac_element_type(self) -> RBACElementType | None:
4950
case AppConfigScopeType.USER:
5051
return RBACElementType.USER
5152

52-
def to_rbac_scope_id(self, scope_id: str) -> str:
53+
def to_rbac_scope_id(self, scope_id: uuid.UUID | None) -> str:
5354
"""The RBAC scope id for a write at this fragment scope.
5455
5556
``public`` is system-wide (no per-entity scope id); ``domain`` / ``user`` carry
56-
their own ``scope_id``.
57+
their own ``scope_id``. RBAC identifies scopes by string, so the owner id is
58+
rendered as text here even though it is stored as a UUID.
5759
"""
58-
return "" if self is AppConfigScopeType.PUBLIC else scope_id
60+
return "" if self is AppConfigScopeType.PUBLIC else str(scope_id)
5961

6062
def default_rank(self) -> int:
6163
"""Default merge rank for an allow-list entry at this scope type (BEP-1052).

src/ai/backend/manager/data/app_config_fragment/types.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from __future__ import annotations
22

3+
import uuid
34
from dataclasses import dataclass
45
from datetime import datetime
56
from typing import Any
@@ -15,7 +16,7 @@ class AppConfigFragmentData:
1516
id: AppConfigFragmentID
1617
config_name: str
1718
scope_type: AppConfigScopeType
18-
scope_id: str
19+
scope_id: uuid.UUID | None
1920
config: dict[str, Any]
2021
created_at: datetime
2122
updated_at: datetime
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
"""convert app_config_fragments.scope_id to a nullable UUID
2+
3+
``scope_id`` held a ``VARCHAR(255)`` that was never really a string: a domain
4+
fragment stores a domain id and a user fragment a user id, both UUIDs, while a
5+
public fragment has no owner at all and stored ``''`` only because the column
6+
was ``NOT NULL``. Store the real type instead — ``UUID NULL``, with ``NULL``
7+
meaning "public, no owner".
8+
9+
The uniqueness of ``(config_name, scope_type, scope_id)`` needs help: Postgres
10+
treats ``NULL``s as distinct in a unique constraint, so the existing constraint
11+
stops rejecting a second public fragment for the same config name once public
12+
rows hold ``NULL``. A partial unique index over the ``NULL`` rows restores the
13+
guarantee the ``''`` sentinel used to provide. (``UNIQUE NULLS NOT DISTINCT``
14+
would say the same thing in one constraint, but it needs Postgres 15+ and the
15+
test fixture runs 13.)
16+
17+
Public rows are forced to ``NULL`` regardless of what they stored, since public
18+
has no owner by definition. Domain and user rows are cast, and a value that is
19+
not a UUID fails the migration on purpose — nulling it would silently drop the
20+
scope binding that decides who can see the fragment.
21+
22+
Revision ID: e5b71c94d2a8
23+
Revises: 577c7a215934
24+
Create Date: 2026-07-21
25+
26+
"""
27+
28+
import sqlalchemy as sa
29+
from alembic import op
30+
31+
# revision identifiers, used by Alembic.
32+
revision = "e5b71c94d2a8"
33+
down_revision = "577c7a215934"
34+
# Part of: NEXT_RELEASE_VERSION
35+
branch_labels = None
36+
depends_on = None
37+
38+
_PUBLIC_INDEX = "uq_app_config_fragments_public_config_name"
39+
_TABLE = "app_config_fragments"
40+
41+
42+
def upgrade() -> None:
43+
op.execute(
44+
sa.text("""
45+
ALTER TABLE app_config_fragments
46+
ALTER COLUMN scope_id DROP NOT NULL,
47+
ALTER COLUMN scope_id TYPE UUID
48+
USING (
49+
CASE
50+
WHEN scope_type = 'public' THEN NULL
51+
ELSE NULLIF(scope_id, '')::uuid
52+
END
53+
)
54+
""")
55+
)
56+
op.create_index(
57+
_PUBLIC_INDEX,
58+
_TABLE,
59+
["config_name", "scope_type"],
60+
unique=True,
61+
postgresql_where=sa.text("scope_id IS NULL"),
62+
)
63+
64+
65+
def downgrade() -> None:
66+
op.drop_index(_PUBLIC_INDEX, table_name=_TABLE)
67+
# Public rows go back to the empty sentinel the NOT NULL column used to require, which
68+
# the plain constraint can compare like any other value.
69+
op.execute(
70+
sa.text("""
71+
ALTER TABLE app_config_fragments
72+
ALTER COLUMN scope_id TYPE VARCHAR(255)
73+
USING (COALESCE(scope_id::text, ''))
74+
""")
75+
)
76+
op.alter_column(_TABLE, "scope_id", nullable=False)

src/ai/backend/manager/models/app_config_fragment/conditions.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@
1010

1111
from ai.backend.common.data.app_config.types import AppConfigScopeType
1212
from ai.backend.common.data.filter_specs import StringMatchSpec
13+
from ai.backend.common.identifier.domain import DomainID
14+
from ai.backend.common.identifier.user import UserID
1315
from ai.backend.manager.models.app_config_fragment.row import AppConfigFragmentRow
1416
from ai.backend.manager.models.clauses import QueryCondition
1517
from ai.backend.manager.models.condition_utils import make_string_in_factory
@@ -118,7 +120,7 @@ def inner() -> sa.sql.expression.ColumnElement[bool]:
118120
# --- scope_id filter ---
119121

120122
@staticmethod
121-
def by_scope_id_equals(scope_id: str) -> QueryCondition:
123+
def by_scope_id_equals(scope_id: uuid.UUID) -> QueryCondition:
122124
def inner() -> sa.sql.expression.ColumnElement[bool]:
123125
return AppConfigFragmentRow.scope_id == scope_id
124126

@@ -136,19 +138,19 @@ def inner() -> sa.sql.expression.ColumnElement[bool]:
136138
return inner
137139

138140
@staticmethod
139-
def by_domain_visibility(domain: str) -> QueryCondition:
140-
"""The ``domain`` scope for ``domain``."""
141+
def by_domain_visibility(domain_id: DomainID) -> QueryCondition:
142+
"""The ``domain`` scope for ``domain_id``."""
141143

142144
def inner() -> sa.sql.expression.ColumnElement[bool]:
143145
return sa.and_(
144146
AppConfigFragmentRow.scope_type == AppConfigScopeType.DOMAIN,
145-
AppConfigFragmentRow.scope_id == domain,
147+
AppConfigFragmentRow.scope_id == domain_id,
146148
)
147149

148150
return inner
149151

150152
@staticmethod
151-
def by_user_visibility(user_id: str) -> QueryCondition:
153+
def by_user_visibility(user_id: UserID) -> QueryCondition:
152154
"""The ``user`` scope for ``user_id``."""
153155

154156
def inner() -> sa.sql.expression.ColumnElement[bool]:

src/ai/backend/manager/models/app_config_fragment/row.py

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
from __future__ import annotations
22

3+
import uuid
34
from typing import Any
45

56
import sqlalchemy as sa
@@ -32,6 +33,17 @@ class AppConfigFragmentRow(LifecycleTimestampsMixin, Base): # type: ignore[misc
3233
"scope_id",
3334
name="uq_app_config_fragments_config_name_scope_type_scope_id",
3435
),
36+
# The constraint above only covers domain and user fragments: Postgres counts NULLs
37+
# as distinct, so it would let a config take any number of public fragments. This
38+
# partial index restores that guarantee for the NULL (public) rows. It replaces the
39+
# NULLS NOT DISTINCT the constraint would otherwise want, which needs Postgres 15+.
40+
sa.Index(
41+
"uq_app_config_fragments_public_config_name",
42+
"config_name",
43+
"scope_type",
44+
unique=True,
45+
postgresql_where=sa.text("scope_id IS NULL"),
46+
),
3547
sa.ForeignKeyConstraint(
3648
["config_name", "scope_type"],
3749
["app_config_allow_list.config_name", "app_config_allow_list.scope_type"],
@@ -56,10 +68,12 @@ class AppConfigFragmentRow(LifecycleTimestampsMixin, Base): # type: ignore[misc
5668
StrEnumType(AppConfigScopeType),
5769
nullable=False,
5870
)
59-
scope_id: Mapped[str] = mapped_column(
71+
# NULL is the public scope: it has no owner. Domain and user fragments carry the id of
72+
# the domain or user that owns them.
73+
scope_id: Mapped[uuid.UUID | None] = mapped_column(
6074
"scope_id",
61-
sa.String(length=255),
62-
nullable=False,
75+
GUID,
76+
nullable=True,
6377
)
6478
config: Mapped[dict[str, Any]] = mapped_column(
6579
"config",

src/ai/backend/manager/repositories/app_config_fragment/creators.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
from __future__ import annotations
44

5+
import uuid
56
from collections.abc import Sequence
67
from dataclasses import dataclass
78
from typing import Any, override
@@ -23,7 +24,7 @@ class AppConfigFragmentCreatorSpec(CreatorSpec[AppConfigFragmentRow]):
2324

2425
config_name: str
2526
scope_type: AppConfigScopeType
26-
scope_id: str
27+
scope_id: uuid.UUID | None
2728
config: dict[str, Any]
2829

2930
@property

src/ai/backend/manager/repositories/app_config_fragment/db_source/db_source.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,9 @@ async def create(self, spec: AppConfigFragmentCreatorSpec) -> AppConfigFragmentD
8585
spec=spec,
8686
element_type=RBACElementType.APP_CONFIG_FRAGMENT,
8787
scope_ref=(
88-
RBACElementRef(element_type, spec.scope_id) if element_type is not None else None
88+
RBACElementRef(element_type, str(spec.scope_id))
89+
if element_type is not None
90+
else None
8991
),
9092
)
9193
async with self._rbac_ops_provider.write_ops() as w:
@@ -223,8 +225,8 @@ async def list_visible_fragments_bulk(
223225
scope_visibility = [AppConfigFragmentConditions.by_public_visibility()]
224226
if scope is not None:
225227
scope_visibility += [
226-
AppConfigFragmentConditions.by_domain_visibility(str(scope.domain_id)),
227-
AppConfigFragmentConditions.by_user_visibility(str(scope.user_id)),
228+
AppConfigFragmentConditions.by_domain_visibility(scope.domain_id),
229+
AppConfigFragmentConditions.by_user_visibility(scope.user_id),
228230
]
229231
querier = BatchQuerier(
230232
pagination=NoPagination(),

src/ai/backend/manager/repositories/app_config_fragment/types.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ def to_condition(self) -> QueryCondition:
6363
def inner() -> sa.sql.expression.ColumnElement[bool]:
6464
return sa.and_(
6565
AppConfigFragmentRow.scope_type == AppConfigScopeType.DOMAIN,
66-
AppConfigFragmentRow.scope_id == str(domain_id),
66+
AppConfigFragmentRow.scope_id == domain_id,
6767
)
6868

6969
return inner
@@ -87,7 +87,7 @@ def to_condition(self) -> QueryCondition:
8787
def inner() -> sa.sql.expression.ColumnElement[bool]:
8888
return sa.and_(
8989
AppConfigFragmentRow.scope_type == AppConfigScopeType.USER,
90-
AppConfigFragmentRow.scope_id == str(user_id),
90+
AppConfigFragmentRow.scope_id == user_id,
9191
)
9292

9393
return inner

src/ai/backend/manager/services/app_config_fragment/actions/create.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,8 @@ def target_element(self) -> RBACElementRef:
4747
element = self.creator_spec.scope_type.to_rbac_element_type()
4848
if element is None:
4949
return RBACElementRef(RBACElementType.APP_CONFIG_FRAGMENT, "")
50-
return RBACElementRef(element, self.creator_spec.scope_id)
50+
# A non-null element type means domain or user scope, which always carries an owner.
51+
return RBACElementRef(element, str(self.creator_spec.scope_id))
5152

5253

5354
@dataclass

0 commit comments

Comments
 (0)