Skip to content

Commit d3e3e91

Browse files
jopemachineclaude
andcommitted
refactor(BA-5827): drop premature error type, switch admin_repo to Updater/Purger, rename extra_config
- Drop `AppConfigFragmentPolicyMissing` (and the FK-violation integrity_error_check that mapped to it). The required-policy invariant is enforced upstream by the service layer; the DB-level FK is the defense-in-depth backstop and now surfaces as a generic `ForeignKeyViolationError`. The typed exception was premature for the DB+repository foundation slice — re-introduce later if a service/API layer needs it. - Refactor the admin_repository / db_source split: db_source now accepts pre-built `Updater` / `Purger` objects (mirroring `scaling_group`, `vfolder`, etc.), with the natural-key → PK resolution exposed as `resolve_pk_by_key`. `admin_repository` builds the `Updater` / `Purger` and translates the missing-row case into `AppConfigFragmentNotFound`. - Rename the JSONB column / Row attribute / Data field / Creator+Updater spec / admin-repository parameter from `extra_config` to `config` end-to-end (DB column included). The legacy-drop migration's downgrade keeps `extra_config` because it recreates the historical schema. - Update the regression test to expect the generic `ForeignKeyViolationError`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 0eda58e commit d3e3e91

9 files changed

Lines changed: 114 additions & 146 deletions

File tree

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ class AppConfigFragmentData:
3030
scope_type: AppConfigScopeType
3131
scope_id: str
3232
name: str
33-
extra_config: Mapping[str, Any] | None
33+
config: Mapping[str, Any] | None
3434
created_at: datetime
3535
updated_at: datetime | None
3636

src/ai/backend/manager/errors/app_config.py

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -56,22 +56,3 @@ def error_code(self) -> ErrorCode:
5656
operation=ErrorOperation.READ,
5757
error_detail=ErrorDetail.NOT_FOUND,
5858
)
59-
60-
61-
class AppConfigFragmentPolicyMissing(BackendAIError, web.HTTPConflict):
62-
"""Raised when a fragment references a `name` without a matching policy row.
63-
64-
Defense-in-depth against the required-policy invariant — normally
65-
the service layer rejects earlier, but the FK violation surfaces
66-
here if the service check is bypassed.
67-
"""
68-
69-
error_type = "https://api.backend.ai/probs/app-config-fragment-policy-missing"
70-
error_title = "Referenced app-config policy does not exist for this fragment."
71-
72-
def error_code(self) -> ErrorCode:
73-
return ErrorCode(
74-
domain=ErrorDomain.BACKENDAI,
75-
operation=ErrorOperation.CREATE,
76-
error_detail=ErrorDetail.CONFLICT,
77-
)

src/ai/backend/manager/models/alembic/versions/a662131d5603_add_app_config_fragments.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ def upgrade() -> None:
5353
nullable=False,
5454
),
5555
sa.Column(
56-
"extra_config",
56+
"config",
5757
pgsql.JSONB(),
5858
nullable=True,
5959
),

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,8 @@ class AppConfigFragmentRow(Base): # type: ignore[misc]
5252
),
5353
nullable=False,
5454
)
55-
extra_config: Mapped[Mapping[str, Any] | None] = mapped_column(
56-
"extra_config",
55+
config: Mapped[Mapping[str, Any] | None] = mapped_column(
56+
"config",
5757
pgsql.JSONB,
5858
nullable=True,
5959
)
@@ -76,7 +76,7 @@ def to_data(self) -> AppConfigFragmentData:
7676
scope_type=self.scope_type,
7777
scope_id=self.scope_id,
7878
name=self.name,
79-
extra_config=dict(self.extra_config) if self.extra_config is not None else None,
79+
config=dict(self.config) if self.config is not None else None,
8080
created_at=self.created_at,
8181
updated_at=self.updated_at,
8282
)

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

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
AppConfigFragmentKey,
1515
AppConfigFragmentSearchResult,
1616
)
17+
from ai.backend.manager.errors.app_config import AppConfigFragmentNotFound
1718
from ai.backend.manager.models.app_config_fragment.row import AppConfigFragmentRow
1819
from ai.backend.manager.models.utils import ExtendedAsyncSAEngine
1920
from ai.backend.manager.repositories.app_config_fragment.creators import (
@@ -26,7 +27,9 @@
2627
AppConfigFragmentUpdaterSpec,
2728
)
2829
from ai.backend.manager.repositories.base.creator import Creator
30+
from ai.backend.manager.repositories.base.purger import Purger
2931
from ai.backend.manager.repositories.base.querier import BatchQuerier
32+
from ai.backend.manager.repositories.base.updater import Updater
3033

3134
app_config_fragment_admin_repository_resilience = Resilience(
3235
policies=[
@@ -48,19 +51,29 @@
4851
)
4952

5053

54+
def _missing(key: AppConfigFragmentKey) -> AppConfigFragmentNotFound:
55+
return AppConfigFragmentNotFound(
56+
extra_msg=(
57+
f"scope_type={key.scope_type.value!r}, scope_id={key.scope_id!r}, name={key.name!r}"
58+
),
59+
)
60+
61+
5162
class AppConfigFragmentAdminRepository:
5263
"""Admin-only operations on AppConfigFragment.
5364
5465
All mutations (`create` / `update` / `purge`) and cross-scope
5566
reads (`admin_search` raw, `admin_search_app_configs` merged)
5667
live here — read-side scope-bound operations are on
5768
`AppConfigFragmentRepository`. Authorization is enforced at the
58-
service layer before reaching either repository.
59-
60-
The required-policy invariant is enforced by the service layer;
61-
the FK on `app_config_fragments.name` is the defense-in-depth
62-
backstop, translated by the creator spec into
63-
:class:`AppConfigFragmentPolicyMissing`.
69+
service layer before reaching either repository. The required-
70+
policy invariant is enforced upstream by the service layer; the
71+
DB-level FK on ``app_config_fragments.name`` is the defense-in-
72+
depth backstop and surfaces as a generic integrity error.
73+
74+
Mutations are routed through the shared Creator / Updater / Purger
75+
helpers so the same execution / resilience plumbing applies as in
76+
sister repositories.
6477
"""
6578

6679
_db_source: AppConfigFragmentDBSource
@@ -74,14 +87,14 @@ def __init__(self, db: ExtendedAsyncSAEngine) -> None:
7487
async def create(
7588
self,
7689
key: AppConfigFragmentKey,
77-
extra_config: Mapping[str, Any],
90+
config: Mapping[str, Any],
7891
) -> AppConfigFragmentData:
7992
creator: Creator[AppConfigFragmentRow] = Creator(
8093
spec=AppConfigFragmentCreatorSpec(
8194
scope_type=key.scope_type,
8295
scope_id=key.scope_id,
8396
name=key.name,
84-
extra_config=extra_config,
97+
config=config,
8598
),
8699
)
87100
return await self._db_source.create(creator)
@@ -90,16 +103,37 @@ async def create(
90103
async def update(
91104
self,
92105
key: AppConfigFragmentKey,
93-
extra_config: Mapping[str, Any],
106+
config: Mapping[str, Any],
94107
) -> AppConfigFragmentData:
95-
"""Update a fragment by natural key. Raises
96-
``AppConfigFragmentNotFound`` when missing."""
97-
spec = AppConfigFragmentUpdaterSpec(extra_config=extra_config)
98-
return await self._db_source.update(key, spec)
108+
"""Update a fragment by natural key. Resolves the natural key
109+
to the row's UUID, builds an ``Updater``, and delegates to the
110+
DB source. Raises :class:`AppConfigFragmentNotFound` when the
111+
row is missing (or vanishes between resolve and write)."""
112+
pk_value = await self._db_source.resolve_pk_by_key(key)
113+
if pk_value is None:
114+
raise _missing(key)
115+
updater: Updater[AppConfigFragmentRow] = Updater(
116+
spec=AppConfigFragmentUpdaterSpec(config=config),
117+
pk_value=pk_value,
118+
)
119+
result = await self._db_source.update(updater)
120+
if result is None:
121+
raise _missing(key)
122+
return result
99123

100124
@app_config_fragment_admin_repository_resilience.apply()
101125
async def purge(self, key: AppConfigFragmentKey) -> bool:
102-
return await self._db_source.purge(key)
126+
"""Delete a fragment by natural key. Resolves the natural key,
127+
builds a ``Purger``, and delegates to the DB source. Returns
128+
``True`` only when a row was actually removed."""
129+
pk_value = await self._db_source.resolve_pk_by_key(key)
130+
if pk_value is None:
131+
return False
132+
purger: Purger[AppConfigFragmentRow] = Purger(
133+
row_class=AppConfigFragmentRow,
134+
pk_value=pk_value,
135+
)
136+
return await self._db_source.purge(purger)
103137

104138
# ── Cross-scope reads ────────────────────────────────────────
105139

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

Lines changed: 9 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,8 @@
66
from dataclasses import dataclass
77
from typing import Any, override
88

9-
from ai.backend.manager.errors.app_config import (
10-
AppConfigFragmentConflict,
11-
AppConfigFragmentPolicyMissing,
12-
)
13-
from ai.backend.manager.errors.repository import (
14-
ForeignKeyViolationError,
15-
UniqueConstraintViolationError,
16-
)
9+
from ai.backend.manager.errors.app_config import AppConfigFragmentConflict
10+
from ai.backend.manager.errors.repository import UniqueConstraintViolationError
1711
from ai.backend.manager.models.app_config_fragment.row import AppConfigFragmentRow
1812
from ai.backend.manager.repositories.base.creator import CreatorSpec
1913
from ai.backend.manager.repositories.base.types import IntegrityErrorCheck
@@ -23,17 +17,17 @@
2317
class AppConfigFragmentCreatorSpec(CreatorSpec[AppConfigFragmentRow]):
2418
"""CreatorSpec for `app_config_fragments`.
2519
26-
Maps DB constraint violations onto typed domain errors:
27-
- ``(scope_type, scope_id, name)`` UNIQUE → :class:`AppConfigFragmentConflict`
28-
- ``name`` FK to `app_config_policies.config_name` →
29-
:class:`AppConfigFragmentPolicyMissing` (required-policy invariant
30-
enforced as defense-in-depth).
20+
Maps the natural-key UNIQUE violation to a typed domain error
21+
(:class:`AppConfigFragmentConflict`). The required-policy
22+
invariant (FK on ``name``) is enforced upstream by the service
23+
layer; the DB-level FK violation surfaces as a generic
24+
integrity error here.
3125
"""
3226

3327
scope_type: str
3428
scope_id: str
3529
name: str
36-
extra_config: Mapping[str, Any]
30+
config: Mapping[str, Any]
3731

3832
@property
3933
@override
@@ -47,12 +41,6 @@ def integrity_error_checks(self) -> Sequence[IntegrityErrorCheck]:
4741
),
4842
),
4943
),
50-
IntegrityErrorCheck(
51-
violation_type=ForeignKeyViolationError,
52-
error=AppConfigFragmentPolicyMissing(
53-
extra_msg=f"No app_config_policies row for name={self.name}",
54-
),
55-
),
5644
)
5745

5846
@override
@@ -61,5 +49,5 @@ def build_row(self) -> AppConfigFragmentRow:
6149
scope_type=self.scope_type,
6250
scope_id=self.scope_id,
6351
name=self.name,
64-
extra_config=dict(self.extra_config),
52+
config=dict(self.config),
6553
)

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

Lines changed: 27 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
AppConfigFragmentSearchResult,
2121
AppConfigScopeType,
2222
)
23-
from ai.backend.manager.errors.app_config import AppConfigFragmentNotFound
2423
from ai.backend.manager.models.app_config_fragment.row import AppConfigFragmentRow
2524
from ai.backend.manager.models.app_config_policy.row import AppConfigPolicyRow
2625
from ai.backend.manager.models.user import UserRow
@@ -29,9 +28,6 @@
2928
AppConfigFragmentSearchScope,
3029
UserAppConfigSearchScope,
3130
)
32-
from ai.backend.manager.repositories.app_config_fragment.updaters import (
33-
AppConfigFragmentUpdaterSpec,
34-
)
3531
from ai.backend.manager.repositories.base.creator import Creator, execute_creator
3632
from ai.backend.manager.repositories.base.purger import Purger, execute_purger
3733
from ai.backend.manager.repositories.base.querier import BatchQuerier, execute_batch_querier
@@ -111,76 +107,48 @@ async def get_by_id(self, id: uuid.UUID) -> AppConfigFragmentData | None:
111107
async def create(self, creator: Creator[AppConfigFragmentRow]) -> AppConfigFragmentData:
112108
"""Insert a new fragment via the shared Creator helper.
113109
114-
Constraint violations translate to typed domain errors via the
115-
spec's ``integrity_error_checks`` (duplicate key →
116-
:class:`AppConfigFragmentConflict`, missing-policy FK →
117-
:class:`AppConfigFragmentPolicyMissing`).
110+
The natural-key UNIQUE violation translates to
111+
:class:`AppConfigFragmentConflict` via the spec's
112+
``integrity_error_checks``. The required-policy invariant
113+
(FK on ``name``) is enforced upstream by the service layer;
114+
a bypass here surfaces as a generic integrity error.
118115
"""
119116
async with self._db.begin_session() as db_sess:
120117
result = await execute_creator(db_sess, creator)
121118
return result.row.to_data()
122119

123120
@app_config_fragment_db_source_resilience.apply()
124-
async def update(
121+
async def resolve_pk_by_key(
125122
self,
126123
key: AppConfigFragmentKey,
127-
spec: AppConfigFragmentUpdaterSpec,
128-
) -> AppConfigFragmentData:
129-
"""Update the fragment identified by its natural key.
130-
131-
The natural key ``(scope_type, scope_id, name)`` is not the PK,
132-
so we first resolve it to the row's UUID `id` and then delegate
133-
to the shared Updater helper (single-column PK required). Raises
134-
:class:`AppConfigFragmentNotFound` when no row exists for the
135-
key (or vanishes between resolve and write); reads use the
136-
nullable `get(...)` instead.
137-
"""
138-
139-
def _missing() -> AppConfigFragmentNotFound:
140-
return AppConfigFragmentNotFound(
141-
extra_msg=(
142-
f"scope_type={key.scope_type.value!r}, "
143-
f"scope_id={key.scope_id!r}, name={key.name!r}"
144-
),
145-
)
146-
147-
async with self._db.begin_session() as db_sess:
148-
pk_value = await db_sess.scalar(
124+
) -> uuid.UUID | None:
125+
"""Resolve the natural key ``(scope_type, scope_id, name)`` to
126+
the row's UUID ``id``. Returns ``None`` when no row matches —
127+
callers translate to a domain-appropriate response."""
128+
async with self._db.begin_readonly_session() as db_sess:
129+
pk: uuid.UUID | None = await db_sess.scalar(
149130
sa.select(AppConfigFragmentRow.id).where(
150131
AppConfigFragmentRow.scope_type == key.scope_type,
151132
AppConfigFragmentRow.scope_id == key.scope_id,
152133
AppConfigFragmentRow.name == key.name,
153134
)
154135
)
155-
if pk_value is None:
156-
raise _missing()
157-
updater: Updater[AppConfigFragmentRow] = Updater(spec=spec, pk_value=pk_value)
158-
result = await execute_updater(db_sess, updater)
159-
if result is None:
160-
raise _missing()
161-
return result.row.to_data()
136+
return pk
162137

163138
@app_config_fragment_db_source_resilience.apply()
164-
async def purge(self, key: AppConfigFragmentKey) -> bool:
165-
"""Delete the fragment identified by the natural key.
139+
async def update(self, updater: Updater[AppConfigFragmentRow]) -> AppConfigFragmentData | None:
140+
"""Apply a pre-built Updater. Returns ``None`` when the row
141+
vanished between PK resolution and write; the caller maps this
142+
to :class:`AppConfigFragmentNotFound`."""
143+
async with self._db.begin_session() as db_sess:
144+
result = await execute_updater(db_sess, updater)
145+
return result.row.to_data() if result is not None else None
166146

167-
Resolves the natural key to the row's UUID `id` and delegates
168-
to the shared Purger helper. Returns `True` when a row was
169-
actually removed.
170-
"""
147+
@app_config_fragment_db_source_resilience.apply()
148+
async def purge(self, purger: Purger[AppConfigFragmentRow]) -> bool:
149+
"""Apply a pre-built Purger. Returns ``True`` when a row was
150+
actually removed (``False`` if the row vanished concurrently)."""
171151
async with self._db.begin_session() as db_sess:
172-
pk_value = await db_sess.scalar(
173-
sa.select(AppConfigFragmentRow.id).where(
174-
AppConfigFragmentRow.scope_type == key.scope_type,
175-
AppConfigFragmentRow.scope_id == key.scope_id,
176-
AppConfigFragmentRow.name == key.name,
177-
)
178-
)
179-
if pk_value is None:
180-
return False
181-
purger: Purger[AppConfigFragmentRow] = Purger(
182-
row_class=AppConfigFragmentRow, pk_value=pk_value
183-
)
184152
result = await execute_purger(db_sess, purger)
185153
return result is not None
186154

@@ -228,7 +196,7 @@ def _merge_chain(
228196
chain: Sequence[str],
229197
) -> _MergedChain:
230198
"""Order `rows` by `chain` (low → high) and deep-merge their
231-
`extra_config` in that order. Empty result projects to `None`.
199+
`config` in that order. Empty result projects to `None`.
232200
233201
Shared by the single-doc and search merge methods so both paths
234202
produce the same shape.
@@ -239,9 +207,9 @@ def _merge_chain(
239207
]
240208
merged: Mapping[str, Any] = {}
241209
for row in ordered:
242-
if row.extra_config is None:
210+
if row.config is None:
243211
continue
244-
merged = deep_merge(merged, row.extra_config)
212+
merged = deep_merge(merged, row.config)
245213
return _MergedChain(
246214
fragments=[row.to_data() for row in ordered],
247215
config=(merged or None),

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,12 @@
1414
class AppConfigFragmentUpdaterSpec(UpdaterSpec[AppConfigFragmentRow]):
1515
"""UpdaterSpec for `app_config_fragments`.
1616
17-
Only `extra_config` is mutable — the ``(scope_type, scope_id, name)``
17+
Only `config` is mutable — the ``(scope_type, scope_id, name)``
1818
natural key is fixed; changing any of those is a new row, not an
1919
update.
2020
"""
2121

22-
extra_config: Mapping[str, Any]
22+
config: Mapping[str, Any]
2323

2424
@property
2525
@override
@@ -28,4 +28,4 @@ def row_class(self) -> type[AppConfigFragmentRow]:
2828

2929
@override
3030
def build_values(self) -> dict[str, Any]:
31-
return {"extra_config": dict(self.extra_config)}
31+
return {"config": dict(self.config)}

0 commit comments

Comments
 (0)