Skip to content

refactor(BA-6948): store app_config_fragments.scope_id as a nullable UUID#12984

Merged
jopemachine merged 6 commits into
mainfrom
feat/BA-6948-app-config-fragment-scope-id-uuid
Jul 21, 2026
Merged

refactor(BA-6948): store app_config_fragments.scope_id as a nullable UUID#12984
jopemachine merged 6 commits into
mainfrom
feat/BA-6948-app-config-fragment-scope-id-uuid

Conversation

@jopemachine

@jopemachine jopemachine commented Jul 21, 2026

Copy link
Copy Markdown
Member

📚 Stacked PRs

Part of the AppConfigFragment / AppConfig stack under BEP-1052 (epic BA-5781). Merge in order (bottom-up):

  1. feat(BA-6552): add app_config_fragments DB model and Alembic migration #12306feat(BA-6552): app_config_fragments DB model and Alembic migration
  2. feat(BA-6553): add app_config_fragments repository layer #12307feat(BA-6553): repository layer
  3. refactor(BA-6619): consolidate AppConfigScopeType into common.data (single definition) #12403refactor(BA-6619): consolidate AppConfigScopeType into common.data
  4. refactor(BA-6620): ExistsQuerier ops primitive + AppConfigAllowList.exists #12405refactor(BA-6620): ExistsQuerier + AppConfigAllowList.exists
  5. feat(BA-6554): add app_config_fragment service layer #12358feat(BA-6554): AppConfigFragment service layer
  6. feat(BA-6702): move fragment rank to the allow list with scope defaults #12516feat(BA-6702): move fragment rank to the allow list with scope defaults
  7. feat(BA-6701): expose allow-list rank on the v2 API surface #12517feat(BA-6701): expose allow-list rank on the v2 API surface
  8. feat(BA-6704): cascade app config subtree deletion from the definition #12518feat(BA-6704): cascade app config subtree deletion from the definition
  9. feat(BA-6626): app_config_fragment bulk repository layer #12426feat(BA-6626): app_config_fragment bulk repository layer
  10. feat(BA-6618): app_config_fragment bulk CRUD service layer #12401feat(BA-6618): app_config_fragment bulk CRUD service layer
  11. feat(BA-6810): AppConfigFragment visible-fragments query (repository layer) #12706feat(BA-6810): AppConfigFragment visible-fragments query (repository layer)
  12. feat(BA-6859): bind AppConfig fragments to their RBAC scope on write (repository layer) #12826feat(BA-6859): bind fragments to their RBAC scope on write (repository layer)
  13. feat(BA-6872): seed APP_CONFIG_FRAGMENT permissions into the RBAC role fixture #12837fix(BA-6872): seed APP_CONFIG_FRAGMENT permissions into the RBAC role fixture
  14. chore(BA-6873): drop the dead APP_CONFIG RBAC entity type #12839chore(BA-6873): drop the dead APP_CONFIG RBAC entity type
  15. feat(BA-6555): AppConfig merge engine + resolve service (service layer) #12359feat(BA-6555): AppConfig merge engine + resolve service (service layer)
  16. 👉 refactor(BA-6948): store app_config_fragments.scope_id as a nullable UUID #12984feat(BA-6948): store app_config_fragments.scope_id as a nullable UUID ← you are here
  17. feat(BA-6921): AppConfig fragment write REST v2 API (CRUD) #12928feat(BA-6921): AppConfig fragment write REST v2 API (CRUD)
  18. feat(BA-6922): AppConfig fragment search REST v2 API (admin + scoped) #12930feat(BA-6922): AppConfig fragment search REST v2 API (admin + scoped)
  19. feat(BA-6556): merged AppConfig REST v2 read API #12377feat(BA-6556): merged AppConfig REST v2 read API

Merge in order, bottom-up. Each PR is based on the one below it, so its diff shows only its own layer.

Resolves #12980 (BA-6948)

Summary

  • app_config_fragments.scope_id becomes UUID NULL, with NULL meaning the public scope, which has no owner.
  • scope_id was never really a string: a domain fragment stores a domain id and a user fragment a user id, both UUIDs (DomainID / UserID are NewTypes over uuid.UUID). A public fragment stored '' only because the column was NOT NULL. So search scopes had to compare against str(domain_id), and a malformed scope_id could be persisted into a row no scope query would ever reach.
  • The RBAC boundary still identifies scopes by string, so to_rbac_scope_id takes UUID | None and renders it (empty for public).
  • by_domain_visibility / by_user_visibility now take DomainID / UserID rather than a bare UUID. by_scope_id_equals stays uuid.UUID, since either kind of id can reach it.

The matching API-boundary change (request/response DTOs taking UUID | None) lives in #12928, which creates those files.

Uniqueness — the part worth reviewing closely

Postgres counts NULLs as distinct in a unique constraint. Once public rows hold NULL, the existing UNIQUE (config_name, scope_type, scope_id) silently stops rejecting a second public fragment for the same config name — a guarantee the '' sentinel was providing for free. A partial unique index over the NULL rows restores it:

CREATE UNIQUE INDEX uq_app_config_fragments_public_config_name
    ON app_config_fragments (config_name, scope_type)
    WHERE scope_id IS NULL;

UNIQUE NULLS NOT DISTINCT would express this in one constraint, but it needs Postgres 15+ and the test fixture (ai.backend.testutils.bootstrap) runs postgres:13.6-alpine. The partial index works on both.

Migration

  • Public rows are forced to NULL regardless of what they stored, since public has no owner by definition.
  • A domain/user scope_id that is not a UUID fails the migration rather than being nulled — that binding decides who can see the fragment, so dropping it silently would be worse than stopping.

Test plan

  • pants lint / pants check clean
  • pants test — repository (real DB) and service tests for app_config_fragment pass
  • Migration verified per the data-migration rule in alembic/AGENTS.md: seeded one row per scope type, ran upgrade, confirmed converted values and column types, confirmed a duplicate public insert is rejected by the partial index, then ran downgrade and confirmed values, nullability, default and index all return to their original shape
  • Confirmed the migration aborts with a clear error on a non-UUID domain scope_id instead of nulling it
  • Live-server verification of create/read with a UUID scope_id

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings July 21, 2026 05:10
@jopemachine
jopemachine requested a review from a team as a code owner July 21, 2026 05:10
@github-actions github-actions Bot added size:L 100~500 LoC comp:manager Related to Manager component comp:common Related to Common component require:db-migration Automatically set when alembic migrations are added or updated labels Jul 21, 2026
@jopemachine
jopemachine force-pushed the feat/BA-6948-app-config-fragment-scope-id-uuid branch from df4fe1e to 69fcb45 Compare July 21, 2026 05:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Converts AppConfig fragment scope ownership from string sentinels to nullable UUIDs across the model, repository, service, and RBAC boundaries.

Changes:

  • Adds UUID migration and public-scope uniqueness index.
  • Propagates nullable UUID typing through application layers.
  • Updates repository tests and release notes.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/unit/manager/repositories/app_config_fragment/test_repository.py Updates fixtures and assertions for UUID scope IDs.
src/ai/backend/manager/services/app_config_fragment/actions/create.py Converts UUID owners to RBAC string identifiers.
src/ai/backend/manager/repositories/app_config_fragment/types.py Uses typed UUID identifiers in visibility conditions.
src/ai/backend/manager/repositories/app_config_fragment/db_source/db_source.py Updates RBAC binding and visibility queries.
src/ai/backend/manager/repositories/app_config_fragment/creators.py Makes creator scope IDs nullable UUIDs.
src/ai/backend/manager/models/app_config_fragment/row.py Changes the column type and adds public uniqueness indexing.
src/ai/backend/manager/models/app_config_fragment/conditions.py Updates scope filters to UUID-based types.
src/ai/backend/manager/models/alembic/versions/e5b71c94d2a8_app_config_fragment_scope_id_to_uuid.py Migrates existing scope IDs to nullable UUIDs.
src/ai/backend/manager/models/alembic/versions/c7e2b48a15d9_merge_a1c4_and_b988.py Reconciles the two Alembic heads.
src/ai/backend/manager/data/app_config_fragment/types.py Updates repository data typing.
src/ai/backend/common/data/app_config/types.py Converts UUID scope IDs at the RBAC boundary.
changes/12982.feature.md Adds the release-note fragment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/ai/backend/manager/models/app_config_fragment/row.py
Comment thread src/ai/backend/manager/models/app_config_fragment/row.py
@jopemachine
jopemachine force-pushed the feat/BA-6948-app-config-fragment-scope-id-uuid branch from 69fcb45 to 7c50d93 Compare July 21, 2026 05:16
@jopemachine
jopemachine marked this pull request as draft July 21, 2026 05:23
@jopemachine
jopemachine force-pushed the feat/BA-6948-app-config-fragment-scope-id-uuid branch from 49be602 to 1d41686 Compare July 21, 2026 05:51
jopemachine and others added 3 commits July 21, 2026 14:54
…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>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@jopemachine
jopemachine force-pushed the feat/BA-6948-app-config-fragment-scope-id-uuid branch from 1d41686 to 9c62736 Compare July 21, 2026 06:00
@jopemachine jopemachine changed the title feat(BA-6948): store app_config_fragments.scope_id as a nullable UUID refactor(BA-6948): store app_config_fragments.scope_id as a nullable UUID Jul 21, 2026
@jopemachine
jopemachine marked this pull request as ready for review July 21, 2026 07:04
jopemachine and others added 3 commits July 21, 2026 16:31
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) <noreply@anthropic.com>
…ckage

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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@jopemachine
jopemachine force-pushed the feat/BA-6948-app-config-fragment-scope-id-uuid branch from 3077a41 to 19ed7e6 Compare July 21, 2026 09:16
@jopemachine
jopemachine merged commit c768721 into main Jul 21, 2026
36 checks passed
@jopemachine
jopemachine deleted the feat/BA-6948-app-config-fragment-scope-id-uuid branch July 21, 2026 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp:common Related to Common component comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated size:L 100~500 LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Store app_config_fragments.scope_id as a nullable UUID

3 participants