Send only the changed permissions and batch the permission state checks - #26127
Conversation
- Post the changed permission names from the MVC modal instead of the whole tree - Replace the per-name list scans with dictionary lookups on the read path
There was a problem hiding this comment.
🟡 Changes recommended
The new “only changed permissions” post path can produce an empty update set, but the current implementation still triggers an update and cache reset, and one of the new tests doesn’t fully assert batching behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR optimizes permission management flows by reducing what the UI submits on save (changed permissions only) and by batching/accelerating permission state checks and grant lookups to avoid per-permission scans and repeated checks.
Changes:
- Razor Pages permission modal now posts only changed permission names (granted/revoked) instead of the full permission tree.
- Permission read/check paths batch simple state checker evaluations and replace repeated list scans with dictionary/HashSet lookups.
- Adds/updates tests and test infrastructure to validate batching and provider reporting behavior.
File summaries
| File | Description |
|---|---|
| modules/permission-management/test/Volo.Abp.PermissionManagement.Domain.Tests/Volo/Abp/PermissionManagement/TestGlobalPermissionStateChecker.cs | Adds a batch-capable global state checker + counter for verifying batching behavior in tests. |
| modules/permission-management/test/Volo.Abp.PermissionManagement.Domain.Tests/Volo/Abp/PermissionManagement/PermissionStore_Tests.cs | Adds a test ensuring mixed cached/uncached permissions are combined correctly in multi-check. |
| modules/permission-management/test/Volo.Abp.PermissionManagement.Domain.Tests/Volo/Abp/PermissionManagement/PermissionManager_Tests.cs | Adds tests around batched state checking and filtering behavior for multi-get. |
| modules/permission-management/test/Volo.Abp.PermissionManagement.Domain.Tests/Volo/Abp/PermissionManagement/AbpPermissionManagementTestModule.cs | Registers the global permission state checker for the permission-management test module. |
| modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs | Adds “only changed permissions” post mode and parsing helpers for granted/revoked names. |
| modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/permission-management-modal.js | Submits only changed permissions by comparing checked vs defaultChecked and posting hidden fields. |
| modules/permission-management/src/Volo.Abp.PermissionManagement.Domain/Volo/Abp/PermissionManagement/PermissionStore.cs | Replaces repeated FirstOrDefault scans with dictionary lookups when rebuilding cache results. |
| modules/permission-management/src/Volo.Abp.PermissionManagement.Domain/Volo/Abp/PermissionManagement/PermissionManager.cs | Batches state checks and uses a name→result dictionary to avoid repeated list searches. |
| modules/permission-management/src/Volo.Abp.PermissionManagement.Domain/Volo/Abp/PermissionManagement/PermissionManagementProvider.cs | Uses a HashSet of granted names instead of Any() per permission. |
| modules/permission-management/src/Volo.Abp.PermissionManagement.Blazor/Components/PermissionManagementModal.razor.cs | Tracks initially loaded values and submits only changed permissions to the app service. |
| modules/permission-management/src/Volo.Abp.PermissionManagement.Blazor.MudBlazor/Components/PermissionManagementModal.razor.cs | Same “only changed permissions” behavior for the MudBlazor modal variant. |
| modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs | Batches state checks and replaces repeated list scans with a dictionary lookup during list building. |
| modules/identity/test/Volo.Abp.Identity.Domain.Tests/Volo/Abp/Identity/PermissionManager_Tests.cs | Adds coverage for de-duplicating role providers when multiple roles grant the same permission. |
| modules/identity/src/Volo.Abp.PermissionManagement.Domain.Identity/Volo/Abp/PermissionManagement/Identity/RolePermissionManagementProvider.cs | De-duplicates permission grants by name to ensure a single role provider is reported. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## rel-10.7 #26127 +/- ##
============================================
+ Coverage 49.72% 49.74% +0.02%
============================================
Files 3820 3821 +1
Lines 133470 133618 +148
Branches 10102 10114 +12
============================================
+ Hits 66369 66472 +103
- Misses 65084 65130 +46
+ Partials 2017 2016 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Assert that no per-permission state check runs in the batched read test
There was a problem hiding this comment.
🔵 Needs a closer look
The new “changed-permissions” post path should normalize/deduplicate and deterministically resolve conflicting permission names to avoid repeated/conflicting updates from malformed input.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
modules/permission-management/src/Volo.Abp.PermissionManagement.Web/Pages/AbpPermissionManagement/PermissionManagementModal.cshtml.cs:152
- GetChangedPermissions() can produce duplicate/conflicting UpdatePermissionDto entries (e.g., same permission listed in both GrantedPermissionNames and RevokedPermissionNames, or repeated lines), which then results in multiple SetAsync calls for the same permission in PermissionAppService.UpdateAsync. It’s safer to normalize/trim/deduplicate server-side and resolve conflicts deterministically (e.g., last write wins, with revoked overriding granted if both are present).
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, improve performance without altering the external contract, and include targeted test coverage for the new batching/delta behaviors.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
- Keep the posted permission names untrimmed so they still match their definition
Related to #26126
The permission management modals now send only the permissions the user changed, so a normal save no longer submits the whole tree.
The read path batches the permission state checks and replaces the per-name list scans with dictionary lookups.