feat(BA-6177): role preset repository, service, and processor layers#11846
feat(BA-6177): role preset repository, service, and processor layers#11846fregataa wants to merge 11 commits into
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Introduces the role-preset stack (repository, DB source, service, processors, actions, errors, and data types), wires it into the central repository / service / processor registries, and adds to_data() conversions on the ORM rows. Operations cover create, get, search, update, bulk soft-delete/restore/purge, and bulk add/remove of role_permission_presets, all built on the new DBOpsProvider (superadmin-scoped via batch_query_in_global).
Changes:
- New
RolePresetRepository/RolePresetDBSourcewith creator, updater, and purger specs, plus dedicatedRolePresetNotFound/RolePermissionPresetConflicterrors andRolePresetData/RolePermissionPresetDataDTOs. - New
RolePresetServiceandRolePresetProcessorsexposing 9 action processors; actions classify permission-row mutations asUPDATEand preset purge asPURGE. - Registration in
Repositories,Services,Processors, andfactory.create_services/create_processors.
Reviewed changes
Copilot reviewed 26 out of 31 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ai/backend/manager/data/role_preset/types.py | New DTOs for preset and permission-preset rows. |
| src/ai/backend/manager/data/role_preset/init.py | New package marker. |
| src/ai/backend/manager/errors/role_preset.py | New RolePresetNotFound and RolePermissionPresetConflict. |
| src/ai/backend/manager/models/rbac_models/role_preset/row.py | Add to_data() returning RolePresetData. |
| src/ai/backend/manager/models/rbac_models/role_permission_preset/row.py | Add to_data() returning RolePermissionPresetData. |
| src/ai/backend/manager/repositories/role_preset/init.py | New package marker. |
| src/ai/backend/manager/repositories/role_preset/creators.py | RolePresetCreatorSpec, RolePermissionPresetCreatorSpec, dependent spec. |
| src/ai/backend/manager/repositories/role_preset/updaters.py | Single-row and deleted-flag batch updater specs. |
| src/ai/backend/manager/repositories/role_preset/purgers.py | Batch purger specs scoped by IDs. |
| src/ai/backend/manager/repositories/role_preset/db_source/init.py | New package marker. |
| src/ai/backend/manager/repositories/role_preset/db_source/db_source.py | Executes specs against DBOpsProvider; snapshot-then-purge / update-then-refetch flows. |
| src/ai/backend/manager/repositories/role_preset/repository.py | Public repository facade delegating to RolePresetDBSource. |
| src/ai/backend/manager/repositories/role_preset/repositories.py | Repositories container with create(). |
| src/ai/backend/manager/repositories/repositories.py | Register role_preset repositories. |
| src/ai/backend/manager/services/role_preset/init.py | New package marker. |
| src/ai/backend/manager/services/role_preset/actions/init.py | New package marker. |
| src/ai/backend/manager/services/role_preset/actions/base.py | RolePresetAction with EntityType.ROLE. |
| src/ai/backend/manager/services/role_preset/actions/create.py | Create action carrying creator + dependent permission specs. |
| src/ai/backend/manager/services/role_preset/actions/get.py | Get action using Querier. |
| src/ai/backend/manager/services/role_preset/actions/search.py | Search action with BatchQuerier and paginated result. |
| src/ai/backend/manager/services/role_preset/actions/update.py | Update action using Updater. |
| src/ai/backend/manager/services/role_preset/actions/delete.py | Bulk soft-delete action via BatchUpdater. |
| src/ai/backend/manager/services/role_preset/actions/restore.py | Bulk restore action via BatchUpdater. |
| src/ai/backend/manager/services/role_preset/actions/purge.py | Bulk hard-purge action via BatchPurger. |
| src/ai/backend/manager/services/role_preset/actions/bulk_add_permissions.py | Bulk add permission-preset rows. |
| src/ai/backend/manager/services/role_preset/actions/bulk_remove_permissions.py | Bulk remove permission-preset rows. |
| src/ai/backend/manager/services/role_preset/service.py | RolePresetService delegating to repository. |
| src/ai/backend/manager/services/role_preset/processors.py | RolePresetProcessors registering 9 ActionProcessors. |
| src/ai/backend/manager/services/processors.py | Register role_preset in Services / Processors / supported_actions. |
| src/ai/backend/manager/services/factory.py | Construct service and processors. |
| changes/11846.feature.md | Changelog entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
511ae2c to
f071f4d
Compare
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
f071f4d to
be1921a
Compare
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fc12fa6 to
442c4db
Compare
| async def bulk_delete( | ||
| self, | ||
| ids: Sequence[RolePresetID], | ||
| ) -> RolePresetBulkUpdateResult: | ||
| return await self._bulk_set_deleted_flag(ids, deleted=True) | ||
|
|
||
| async def bulk_restore( | ||
| self, | ||
| ids: Sequence[RolePresetID], | ||
| ) -> RolePresetBulkUpdateResult: | ||
| return await self._bulk_set_deleted_flag(ids, deleted=False) | ||
|
|
||
| async def _bulk_set_deleted_flag( | ||
| self, | ||
| ids: Sequence[RolePresetID], | ||
| *, | ||
| deleted: bool, | ||
| ) -> RolePresetBulkUpdateResult: | ||
| spec = RolePresetDeletedFlagUpdaterSpec(deleted=deleted) | ||
| updaters = [Updater(spec=spec, pk_value=preset_id) for preset_id in ids] | ||
| async with self._ops.write_ops() as w: | ||
| result = await w.bulk_update_partial(updaters) | ||
| successes = [row.to_data() for row in result.successes] | ||
| return RolePresetBulkUpdateResult(successes=successes, failures=result.errors) |
There was a problem hiding this comment.
Rather than separating delete and restore operations at the repository level, it seems better to separate them at the service layer and pass different specs.
| async def bulk_purge( | ||
| self, | ||
| ids: Sequence[RolePresetID], | ||
| ) -> RolePresetBulkPurgeResult: | ||
| purgers = [Purger(row_class=RolePresetRow, pk_value=preset_id) for preset_id in ids] | ||
| async with self._ops.write_ops() as w: | ||
| result = await w.bulk_purge_partial(purgers) | ||
| successes = [row.to_data() for row in result.successes] | ||
| failures = [ | ||
| RolePresetPurgeFailure( | ||
| id=ids[error.index], | ||
| message=str(error.exception), | ||
| ) | ||
| for error in result.errors | ||
| ] | ||
| return RolePresetBulkPurgeResult( | ||
| successes=successes, | ||
| failures=failures, | ||
| ) |
There was a problem hiding this comment.
Instead of converting messages at the repository layer, I think it might be better to handle this at the adapter level.
| class BulkRemoveRolePermissionPresetsAction(RolePresetAction): | ||
| ids: Sequence[RolePermissionPresetID] | ||
|
|
||
| @override | ||
| def entity_id(self) -> str | None: | ||
| return None | ||
|
|
||
| @override | ||
| @classmethod | ||
| def operation_type(cls) -> ActionOperationType: | ||
| return ActionOperationType.UPDATE |
There was a problem hiding this comment.
Is there no BulkRolePresetAction?
| @dataclass | ||
| class SearchRolePresetsAction(RolePresetAction): | ||
| querier: BatchQuerier | ||
|
|
||
| @override | ||
| def entity_id(self) -> str | None: | ||
| return None | ||
|
|
||
| @override | ||
| @classmethod | ||
| def operation_type(cls) -> ActionOperationType: | ||
| return ActionOperationType.SEARCH | ||
|
|
There was a problem hiding this comment.
I think it should be a scope action, right?
- Introduce role_preset domain types (data, errors, creators, updaters, purgers). - Implement RolePresetRepository over DBOpsProvider with create, get, search, update, bulk delete/restore/purge, and bulk add/remove of role_permission_preset child rows. - Wire RolePresetService and RolePresetProcessors with Actions that carry their own Creator/Updater/Purger objects so the db_source is a thin executor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- GetRolePresetAction and single PurgeRolePresetAction now take preset_id instead of carrying Querier/BatchPurger specs. - Wrap repository search result in RolePresetSearchResult; create returns RolePresetData (preset only) as the action result no longer carries permission rows. - Add single purge (success/failure) and switch bulk purge to per-id partial deletion returning success_count + per-id failures, via a new DBOpsProvider.bulk_purge_partial. - Drop unused RolePresetByIDsBatchPurgerSpec. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- bulk_delete / bulk_restore now return updated_count instead of refetching rows; drop the fragile _batch_update_and_refetch helper (re-running update conditions could miss just-mutated rows). - bulk_remove_permissions returns the removed count instead of row snapshots. - Aligns bulk operations with the count-based convention used by the scheduler and deployment domains. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Add tests/unit/manager/repositories/role_preset covering create (with dependent permission rows), id-based get, search, update, soft delete/restore counts, single + bulk purge, and bulk add/remove. - CreateRolePresetAction now carries a bare RolePresetCreatorSpec instead of a pre-wrapped Creator[RolePresetRow], matching the sibling deployment_revision_preset pattern; db_source wraps it internally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…h_remove naming - Add execute_bulk_updater_partial primitive (savepoint per row, silent skip on no match) mirroring execute_bulk_purger_partial; expose WriteOps.bulk_update_partial. - bulk_delete/bulk_restore now accept ids and return RolePresetBulkUpdateResult (successes + BulkUpdaterError failures) instead of a bare count. - Rename bulk_remove_permissions -> batch_remove_permissions: it is a single-SQL batch operation, not a per-row bulk; action and service surfaces align with the base-layer batch/bulk distinction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Align every bulk role-preset / permission operation with the codebase consensus that partial-failure ops return success records plus a failures list: - bulk_purge: return purged RolePresetData snapshots instead of a bare success_count. - bulk_add_permissions: switch from atomic bulk_create to bulk_create_partial and return successes + failures (was a bare list with no failure tracking). - batch_remove_permissions -> bulk_remove_permissions: switch from atomic condition-based batch_purge (count only) to per-id bulk_purge_partial, taking permission ids and returning removed records + failures. Drop the now-unused RolePermissionPresetByIDsBatchPurgerSpec. Also drop the duplicate execute_bulk_updater_partial left over from the rebase onto main (main already provides it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…elete; bulk/scope action bases - bulk_purge: db_source returns bulk_purge_partial errors as-is (records + failures), dropping the custom RolePresetPurgeFailure mapping; ActionResult holds the RolePresetBulkPurgeResult wholesale. - soft delete/restore: actions carry Updater specs directly; repository runs the given updaters via a single bulk_update, no spec construction in db_source. - actions: add RolePresetBulkAction / RolePresetScopeAction lightweight bases so bulk (delete/restore/purge) and scope (search) actions inherit appropriately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
442c4db to
6128925
Compare
Create operates within an RBAC scope rather than on an existing entity, so it inherits RolePresetScopeAction (entity_id -> None) instead of the bare action. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Role permission presets are a distinct entity from role presets, so their bulk actions now inherit a dedicated RolePermissionPresetBulkAction base with the ROLE_PERMISSION entity type instead of the RolePreset (ROLE) action base. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| @dataclass | ||
| class RolePresetScopeAction(RolePresetAction): | ||
| """Base for actions scoped to a collection of role presets (e.g. search). | ||
|
|
||
| Scope operations query within an RBAC scope rather than acting on one | ||
| entity, so there is no single entity id to report. | ||
| """ | ||
|
|
||
| @override | ||
| def entity_id(self) -> str | None: | ||
| return None | ||
|
|
||
|
|
||
| @dataclass | ||
| class RolePermissionPresetAction(BaseAction): | ||
| """Base for actions on role permission presets. | ||
|
|
||
| A role permission preset is a distinct entity from a role preset, so its | ||
| actions form their own hierarchy with the ``ROLE_PERMISSION`` entity type. | ||
| """ | ||
|
|
||
| @override | ||
| @classmethod | ||
| def entity_type(cls) -> EntityType: | ||
| return EntityType.ROLE_PERMISSION | ||
|
|
||
|
|
||
| @dataclass | ||
| class RolePermissionPresetBulkAction(RolePermissionPresetAction): | ||
| """Base for actions that operate on multiple role permission presets at once. | ||
|
|
||
| Bulk operations target a set of permission presets rather than a single | ||
| entity, so there is no single entity id to report. | ||
| """ | ||
|
|
||
| @override | ||
| def entity_id(self) -> str | None: |
There was a problem hiding this comment.
Are "scope actions" and "bulk actions" currently not distinguished from one another?
Summary
RolePresetRepository/RolePresetService/RolePresetProcessorscovering create, get, search, update, bulk delete/restore/purge, and bulk add/remove forrole_permission_presetchild rows.DBOpsProvider(superadmin-scoped viabatch_query_in_global), with soft-delete + restore + hard-purge semantics. Bulk delete/restore useBatchUpdater+ re-fetch; bulk purge/remove pre-fetch snapshots thenBatchPurger.Creator/Updater/BatchUpdater/Purger/BatchPurger/BulkCreator(andQuerierfor get) —db_sourceonly executes, never constructs them.Resolves BA-6177