feat(BA-6555): AppConfig merge engine + resolve service (service layer)#12359
feat(BA-6555): AppConfig merge engine + resolve service (service layer)#12359jopemachine wants to merge 1 commit into
Conversation
b40117b to
870a98b
Compare
296a9a0 to
07bafd2
Compare
1777f48 to
9dc1da7
Compare
47878cb to
ae7f25f
Compare
9dc1da7 to
59a0a70
Compare
e9f0e1b to
684601c
Compare
59a0a70 to
7694f19
Compare
684601c to
e5986d3
Compare
7694f19 to
02106ca
Compare
e5986d3 to
cb34649
Compare
94e2ca4 to
08e919c
Compare
fb7af85 to
4b9ce45
Compare
ab11cd9 to
5e78703
Compare
b0fed49 to
3a9f1a1
Compare
5e78703 to
ce8e7a0
Compare
3a9f1a1 to
43f39c1
Compare
There was a problem hiding this comment.
Pull request overview
This PR (BA-6555) adds the read side of the AppConfig stack under BEP-1052: a service that resolves the merged AppConfig for a (user, config_name) principal by deep-merging the visible public / domain / user fragments in allow-list rank order. It is the read foundation for the upcoming REST v2 API (#12377) and introduces no HTTP surface itself.
Changes:
- New
AppConfigServicewithresolve/resolve_bulkactions, processors, and a_merge_configshelper built oncommon.utils.deep_merge. - New repository/db_source methods
list_visible_fragments/list_visible_fragments_bulk(single OR-filtered query joined to the allow-list forrank), plus per-scope visibility conditions and theAppConfigScopeArgumentsvalue object. - New
AppConfigDatamerged-view type and unit/real-DB tests.
Reviewed changes
Copilot reviewed 14 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
services/app_config/service.py |
Merge engine (_merge_configs) and resolve / resolve_bulk service methods. |
services/app_config/actions/{base,resolve,resolve_bulk}.py |
USER-scope GET actions/results carrying merged AppConfigData. |
services/app_config/processors.py |
Processor package wiring the two resolve actions. |
data/app_config/types.py |
AppConfigData merged-view dataclass. |
repositories/app_config_fragment/repository.py |
Delegates the two visible-fragment reads to the db_source. |
repositories/app_config_fragment/db_source/db_source.py |
Builds the joined, rank-ordered visible-fragments query. |
repositories/app_config_fragment/types.py |
Adds AppConfigScopeArguments. |
models/app_config_fragment/conditions.py |
by_public/domain/user_visibility conditions. |
tests/.../services/app_config/*, tests/.../repositories/app_config_fragment/test_repository.py |
Service + real-DB repository tests. |
changes/12359.feature.md |
Changelog fragment. |
Key points raised: the deep_merge list-merge behavior diverges from the described "wholesale-replace" semantics (and is untested), the config empty-case returns {} rather than the None the description claims, and the "single query" read path actually issues a discarded extra COUNT(*).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
|
|
||
| config_names: list[str] | ||
| scope: AppConfigScopeArguments | None = None |
There was a problem hiding this comment.
Is it assuming that it only handles items within the same scope?
Resolve the merged AppConfig for a (user, config_name) on top of the repository's visible-fragments query: - AppConfigData: the merged per-user view (ordered fragments + deep-merged config; None when no fragment contributes) - deep-merge in rank order — nested dicts recurse, lists/scalars are replaced wholesale by the higher-rank fragment - resolve / resolve_bulk actions + processors (bulk repeats duplicate names in order) - mocked-repository service tests Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
📚 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)feat(BA-6886): registerAPP_CONFIG_FRAGMENTas an RBAC resource type + permission backfillfeat(BA-6859): bind fragments to their RBAC scope on write (repository layer)fix(BA-6872): seedAPP_CONFIG_FRAGMENTpermissions into the RBAC role fixturechore(BA-6873): drop the dead APP_CONFIG RBAC entity typefeat(BA-6555): AppConfig merge engine + resolve service (service layer) ← you are herefeat(BA-6556): AppConfig REST v2 APIfeat(BA-6860): fragment write API routes + bulk write authzAccess model: fragment
create/update/purgeare gated by RBAC (a user writes their ownuser-scope fragment, a domain admin their domain's, a superadmin any —publicis superadmin-only); reads are gated by the allow list (existence = readable layer,rank= merge order) plus scope visibility, with no RBAC on the read path;app_config_definitions/app_config_allow_listthemselves are superadmin-only (REST + GQL). Closed / superseded: #12794 · #12756 · #12764.Summary
Adds the service / merge layer of the merged AppConfig read (BA-6555), on top of the repository query in #12706. Resolves the merged
AppConfigfor a(user, config_name)principal by deep-merging, inrankorder, the fragments that #12706'slist_visible_fragmentsreturns. This is the read foundation the REST/GQL endpoints (BA-6556, #12377) build on.What's included
data/app_config/types.py):AppConfigData— the ordered contributingfragments(rank low → high) plus their deep-mergedmerged_config.merged_configisNonewhen no fragment contributes (a defined-but-unconfigured name), distinct from a fragment that merges to an empty{}.AppConfigService._merge_configs/_deep_merge_replace): fragments deep-merged in ascendingrank— nested dicts recurse, but lists and scalars are replaced wholesale by the higher-rank fragment (a user's list fully overrides the lower scope's, not element-by-index). Intentionally notcommon.utils.deep_merge, which index-merges lists.resolve(oneconfig_name) andresolve_bulk(several names for one principal, one query per distinct scope). Both areUSER-scopeGETactions returningAppConfigDatain request order; a name repeated inresolve_bulkis repeated in the output.resolve/resolve_bulkservice paths against a mocked repository.The HTTP surface (raw fragments + merged read/update) lands in BA-6556 (REST v2, #12377).
📚 Documentation preview 📚: https://sorna--12359.org.readthedocs.build/en/12359/
📚 Documentation preview 📚: https://sorna-ko--12359.org.readthedocs.build/ko/12359/
📚 Documentation preview 📚: https://sorna--12359.org.readthedocs.build/en/12359/
📚 Documentation preview 📚: https://sorna-ko--12359.org.readthedocs.build/ko/12359/
📚 Documentation preview 📚: https://sorna--12359.org.readthedocs.build/en/12359/
📚 Documentation preview 📚: https://sorna-ko--12359.org.readthedocs.build/ko/12359/