Skip to content

Set permissions in a single batch instead of one call per permission - #26126

Open
maliming wants to merge 2 commits into
devfrom
maliming/permission-batch-update
Open

Set permissions in a single batch instead of one call per permission#26126
maliming wants to merge 2 commits into
devfrom
maliming/permission-batch-update

Conversation

@maliming

@maliming maliming commented Sep 1, 2026

Copy link
Copy Markdown
Member

Resolve #26117

PermissionAppService.UpdateAsync queried the current state once per permission, so saving the permission modal ran one database round-trip per submitted permission. It now reads the whole set once and writes only the changed permissions.

DisableTracking was silently ignored by the repositories whose queries start from GetDbSetAsync() instead of GetQueryableAsync(), which made the batched read track every row.

- Honor DisableTracking in the permission, identity, OpenIddict and IdentityServer repositories
- Skip the feature value update when the stored value is unchanged
Copilot AI lite review requested due to automatic review settings September 1, 2026 12:34
@maliming maliming added this to the 10.8-preview milestone Sep 1, 2026

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

This PR improves permission update performance by batching permission state reads (instead of querying once per permission) and ensures EF Core repositories respect DisableTracking by starting queries from GetQueryableAsync(). It also avoids unnecessary writes in feature management when the value hasn’t changed.

Changes:

  • Add a batched SetAsync(IEnumerable<KeyValuePair<string,bool>> ...) flow and use it from PermissionAppService.UpdateAsync.
  • Update several EF Core repositories to query via GetQueryableAsync() (so tracking configuration is honored).
  • Avoid updating FeatureValue when the stored value is unchanged, with added tests for event emission behavior.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
modules/permission-management/test/Volo.Abp.PermissionManagement.TestBase/Volo/Abp/PermissionManagement/TestPermissionManagementProvider.cs Adds call tracking to validate batch read/write behavior in tests.
modules/permission-management/test/Volo.Abp.PermissionManagement.Domain.Tests/Volo/Abp/PermissionManagement/PermissionManager_Tests.cs Adds tests for multi-permission batch set semantics and reduced querying.
modules/permission-management/test/Volo.Abp.PermissionManagement.Application.Tests/Volo/Abp/PermissionManagement/PermissionAppService_Tests.cs Adds application-level test ensuring grants/revokes are applied together and batched checks occur.
modules/permission-management/src/Volo.Abp.PermissionManagement.EntityFrameworkCore/Volo/Abp/PermissionManagement/EntityFrameworkCore/EfCoreResourcePermissionGrantRepository.cs Switches to GetQueryableAsync() to respect tracking configuration.
modules/permission-management/src/Volo.Abp.PermissionManagement.EntityFrameworkCore/Volo/Abp/PermissionManagement/EntityFrameworkCore/EfCorePermissionGrantRepository.cs Switches to GetQueryableAsync() to respect tracking configuration.
modules/permission-management/src/Volo.Abp.PermissionManagement.Domain/Volo/Abp/PermissionManagement/PermissionManager.cs Implements batched permission set logic (single read, write only diffs).
modules/permission-management/src/Volo.Abp.PermissionManagement.Domain/Volo/Abp/PermissionManagement/IPermissionManager.cs Adds new batch SetAsync member to the public interface.
modules/permission-management/src/Volo.Abp.PermissionManagement.Application/Volo/Abp/PermissionManagement/PermissionAppService.cs Uses the new batch API for UpdateAsync instead of per-permission calls.
modules/openiddict/src/Volo.Abp.OpenIddict.EntityFrameworkCore/Volo/Abp/OpenIddict/Applications/EfCoreOpenIddictApplicationRepository.cs Switches to GetQueryableAsync() to respect tracking configuration.
modules/identityserver/src/Volo.Abp.IdentityServer.EntityFrameworkCore/Volo/Abp/IdentityServer/Clients/ClientRepository.cs Switches to GetQueryableAsync() to respect tracking configuration.
modules/identity/src/Volo.Abp.Identity.EntityFrameworkCore/Volo/Abp/Identity/EntityFrameworkCore/EfCoreIdentityUserRepository.cs Switches multiple queries to GetQueryableAsync() to respect tracking configuration.
modules/identity/src/Volo.Abp.Identity.EntityFrameworkCore/Volo/Abp/Identity/EntityFrameworkCore/EfCoreIdentityRoleRepository.cs Switches multiple queries to GetQueryableAsync() to respect tracking configuration.
modules/feature-management/test/Volo.Abp.FeatureManagement.TestBase/Volo/Abp/FeatureManagement/FeatureManagementStore_Tests.cs Adds tests asserting no update event when the value is unchanged and one update event when changed.
modules/feature-management/src/Volo.Abp.FeatureManagement.Domain/Volo/Abp/FeatureManagement/FeatureManagementStore.cs Avoids unnecessary update when the incoming value matches the stored value.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 41.57303% with 208 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.76%. Comparing base (1f7b7a5) to head (64a7b71).
⚠️ Report is 6 commits behind head on dev.

Files with missing lines Patch % Lines
...bp/PermissionManagement/PermissionManager_Tests.cs 0.00% 197 Missing ⚠️
.../FeatureManagement/FeatureManagementStore_Tests.cs 94.11% 3 Missing ⚠️
...olo/Abp/IdentityServer/Clients/ClientRepository.cs 25.00% 3 Missing ⚠️
...ntityFrameworkCore/EfCoreIdentityRoleRepository.cs 66.66% 1 Missing and 1 partial ⚠️
...ntityFrameworkCore/EfCoreIdentityUserRepository.cs 93.33% 1 Missing ⚠️
...lications/EfCoreOpenIddictApplicationRepository.cs 83.33% 0 Missing and 1 partial ⚠️
...Volo/Abp/PermissionManagement/PermissionManager.cs 98.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #26126      +/-   ##
==========================================
- Coverage   49.79%   49.76%   -0.03%     
==========================================
  Files        3830     3830              
  Lines      133966   134269     +303     
  Branches    10156    10163       +7     
==========================================
+ Hits        66709    66821     +112     
- Misses      65247    65442     +195     
+ Partials     2010     2006       -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PermissionAppService.UpdateAsync is very slow

2 participants