feat(BA-6810): AppConfigFragment visible-fragments query (repository layer)#12706
Merged
Conversation
github-actions
Bot
added
size:L
jopemachine
force-pushed
the
feat/BA-6555-app-config-visible-fragments
branch
from
a676c3f to
5816ebe
Compare
jopemachine
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds the repository read layer for resolving a merged AppConfig — the query foundation the merge/resolve service (#12359) builds on. Given a (domain, user) principal, it fetches the fragments visible to that principal (the public scope, plus the principal's own domain and user fragments) for one or many config names, joined to the allow-list to carry the merge rank, in a single rank-ordered query. It is part of the AppConfigFragment/AppConfig stack under BEP-1052.
Changes:
- Adds visibility query conditions (
by_config_names,by_public_visibility,by_domain_visibility,by_user_visibility) kept separate so the read AND-combinesconfig_name IN (...)with the(public OR domain OR user)scope group. - Adds
AppConfigScopeArguments(bundled(domain_id, user_id)principal) and thelist_visible_fragments/list_visible_fragments_bulkmethods on the db_source + repository, using an inner join to the allow-list forrank, rank-ordered. - Adds real-DB repository tests for the visibility conditions and the merged read, plus a changelog fragment.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/ai/backend/manager/models/app_config_fragment/conditions.py |
New name-membership + per-scope visibility condition builders. |
src/ai/backend/manager/repositories/app_config_fragment/types.py |
New AppConfigScopeArguments principal value object. |
src/ai/backend/manager/repositories/app_config_fragment/db_source/db_source.py |
_list_visible_fragments + single/bulk public methods (allow-list join, rank ordering). |
src/ai/backend/manager/repositories/app_config_fragment/repository.py |
Resilience-wrapped repository passthroughs for the two new reads. |
tests/unit/manager/repositories/app_config_fragment/test_repository.py |
Real-DB tests for visibility conditions and merged read. |
changes/12706.feature.md |
Changelog fragment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…layer) Add the rank-ordered read that the merged-AppConfig service builds on: - by_config_names + scope-only by_public/domain/user_visibility conditions - AppConfigScopeArguments (the resolving (domain, user) principal) - list_visible_fragments / list_visible_fragments_bulk: per name, the public OR the scope's domain OR its user fragment, joined to the allow-list for the merge rank, in a single query - real-DB repository tests for the visibility conditions and the merged read Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jopemachine
force-pushed
the
feat/BA-6555-app-config-visible-fragments
branch
from
5816ebe to
81ef72e
Compare
seedspirit
reviewed
Jul 10, 2026
Comment on lines
+218
to
+221
| scope_visibility = [ | ||
| AppConfigFragmentConditions.by_public_visibility(), | ||
| AppConfigFragmentConditions.by_domain_visibility(str(scope.domain_id)), | ||
| AppConfigFragmentConditions.by_user_visibility(str(scope.user_id)), |
Contributor
There was a problem hiding this comment.
It doesn't seem necessary to manage this as a separate variable.
Comment on lines
+138
to
+160
| @staticmethod | ||
| def by_domain_visibility(domain: str) -> QueryCondition: | ||
| """The ``domain`` scope for ``domain``.""" | ||
|
|
||
| def inner() -> sa.sql.expression.ColumnElement[bool]: | ||
| return sa.and_( | ||
| AppConfigFragmentRow.scope_type == AppConfigScopeType.DOMAIN, | ||
| AppConfigFragmentRow.scope_id == domain, | ||
| ) | ||
|
|
||
| return inner | ||
|
|
||
| @staticmethod | ||
| def by_user_visibility(user_id: str) -> QueryCondition: | ||
| """The ``user`` scope for ``user_id``.""" | ||
|
|
||
| def inner() -> sa.sql.expression.ColumnElement[bool]: | ||
| return sa.and_( | ||
| AppConfigFragmentRow.scope_type == AppConfigScopeType.USER, | ||
| AppConfigFragmentRow.scope_id == user_id, | ||
| ) | ||
|
|
||
| return inner |
Contributor
There was a problem hiding this comment.
Did you decide that managing it this way is better than generalizing it into a single condition? (I’m asking just to confirm, since this seems like a design choice.)
Comment on lines
+110
to
+120
| @app_config_fragment_repository_resilience.apply() | ||
| async def list_visible_fragments( | ||
| self, config_name: str, scope: AppConfigScopeArguments | ||
| ) -> list[AppConfigFragmentData]: | ||
| return await self._db_source.list_visible_fragments(config_name, scope) | ||
|
|
||
| @app_config_fragment_repository_resilience.apply() | ||
| async def list_visible_fragments_bulk( | ||
| self, config_names: list[str], scope: AppConfigScopeArguments | ||
| ) -> list[AppConfigFragmentData]: | ||
| return await self._db_source.list_visible_fragments_bulk(config_names, scope) |
Contributor
There was a problem hiding this comment.
It seems like just using bulk would be fine- is there really a need to manage list_visible_fragments separately?
Drop the single-name list_visible_fragments and the shared private _list_visible_fragments helper, inlining the query body into list_visible_fragments_bulk. Callers use the bulk form with a single-element list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines
+231
to
+239
| # Join each fragment to its allow-list entry (indexed ``(config_name, scope_type)`` FK | ||
| # pair), which carries the merge ``rank`` the result is ordered by. | ||
| selector = sa.select(AppConfigFragmentRow).join( | ||
| AppConfigAllowListRow, | ||
| sa.and_( | ||
| AppConfigAllowListRow.config_name == AppConfigFragmentRow.config_name, | ||
| AppConfigAllowListRow.scope_type == AppConfigFragmentRow.scope_type, | ||
| ), | ||
| ) |
Collaborator
There was a problem hiding this comment.
Isn't the "join" unnecessary?
HyeockJinKim
approved these changes
Jul 10, 2026
This was referenced Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📚 Stacked PRs
Part of the AppConfigFragment / AppConfig stack under BEP-1052 (epic BA-5781). Merge in order:
feat(BA-6552): app_config_fragments DB model and Alembic migrationfeat(BA-6553): repository layerrefactor(BA-6619): consolidate AppConfigScopeType into common.datarefactor(BA-6620): ExistsQuerier + AppConfigAllowList.existsfeat(BA-6554): AppConfigFragment service layerfeat(BA-6702): move fragment rank to the allow list with scope defaultsfeat(BA-6701): expose allow-list rank on the v2 API surfacefeat(BA-6704): cascade app config subtree deletion from the definitionfeat(BA-6626): app_config_fragment bulk repository layerfeat(BA-6618): app_config_fragment bulk CRUD service layerfeat(BA-6810): AppConfigFragment visible-fragments query (repository layer)← you are herefeat(BA-6555): AppConfig merge engine + resolve service (service layer)feat(BA-6556): AppConfig REST v2 APISummary
Split out of #12359 (BA-6555). This is the repository read layer the merged-AppConfig service (#12359) builds on; the merge engine + resolve service follows in #12359 (stacked on this).
What's included
models/app_config_fragment/conditions.py):by_config_names(name membership) plus scope-onlyby_public_visibility/by_domain_visibility/by_user_visibility— kept separate so the merged read AND-combinesconfig_name IN (...)with(public OR domain OR user).AppConfigScopeArguments(repositories/app_config_fragment/types.py): the resolving(domain, user)principal, bundled so it travels together.list_visible_fragments/list_visible_fragments_bulk(db_source.py+repository.py): per name, thepublicOR the scope'sdomainOR itsuserfragment, joined to the allow-list for the mergerank, fetched in a single query (rank-ordered, ready to deep-merge).No service/merge logic here — that is #12359.