Respect AllowedProviders/Providers in Feature/Setting management#25462
Conversation
- FeatureManager.SetAsync: throw when providerName is not in feature.AllowedProviders - FeatureManager.GetAllWithProviderAsync / GetOrNullInternalAsync: filter providers chain by feature.AllowedProviders (aligning with FeatureChecker) - FeatureAppService.GetAsync: filter features by AllowedProviders and add parent chain check to prevent orphan child entries (aligning with PermissionAppService) - SettingManager.SetAsync: throw when providerName is not in setting.Providers - SettingManager.GetAllAsync / GetOrNullInternalAsync: filter providers chain by setting.Providers (aligning with SettingProvider)
There was a problem hiding this comment.
Pull request overview
This PR tightens feature/setting management to respect each definition’s allowed provider list: writes reject disallowed providers, and reads filter the provider chain so behavior aligns more closely with IFeatureChecker / ISettingProvider. It also updates the Feature Management UI read model to omit definitions outside the active management provider (and avoids returning orphan children).
Changes:
- Enforce provider compatibility for
SettingManager/FeatureManagerread+write paths (provider-chain filtering + write-time rejection). - Update
FeatureAppService.GetAsyncto filter out disallowed features (and orphan children) for the current management provider. - Add/extend test definitions and test cases covering allowed/disallowed provider scenarios and stale-fallback edge cases.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/setting-management/test/Volo.Abp.SettingManagement.Tests/Volo/Abp/SettingManagement/SettingManager_Basic_Tests.cs | Adds tests for disallowed provider writes and read filtering behavior in setting management. |
| modules/setting-management/test/Volo.Abp.SettingManagement.TestBase/Volo/Abp/SettingManagement/TestSettingDefinitionProvider.cs | Introduces settings restricted to specific providers for test coverage. |
| modules/setting-management/src/Volo.Abp.SettingManagement.Domain/Volo/Abp/SettingManagement/SettingManager.cs | Filters provider chains by SettingDefinition.Providers and rejects disallowed provider writes. |
| modules/feature-management/test/Volo.Abp.FeatureManagement.TestBase/Volo/Abp/FeatureManagement/TestFeatureDefinitionProvider.cs | Adds provider-restricted feature definitions (including parent/child cases) for tests. |
| modules/feature-management/test/Volo.Abp.FeatureManagement.Domain.Tests/Volo/Abp/FeatureManagement/FeatureManager_Tests.cs | Adds tests for disallowed provider writes and filtered read behavior in FeatureManager. |
| modules/feature-management/test/Volo.Abp.FeatureManagement.Application.Tests/Volo/Abp/FeatureManagement/FeatureAppService_Tests.cs | Adds tests asserting the app service hides disallowed features and orphan children. |
| modules/feature-management/src/Volo.Abp.FeatureManagement.Domain/Volo/Abp/FeatureManagement/FeatureManager.cs | Filters provider chains by FeatureDefinition.AllowedProviders and rejects disallowed provider writes. |
| modules/feature-management/src/Volo.Abp.FeatureManagement.Application/Volo/Abp/FeatureManagement/FeatureAppService.cs | Filters features returned from GetAsync based on management provider and parent inclusion. |
Comments suppressed due to low confidence (2)
modules/setting-management/src/Volo.Abp.SettingManagement.Domain/Volo/Abp/SettingManagement/SettingManager.cs:144
- The new provider compatibility exception message is grammatically incorrect ("has not compatible") and is also evaluated before checking whether the provider actually exists. For a restricted setting, passing an unknown
providerNamewill now throw the compatibility exception instead of the existing "Unknown setting value provider" error, which can hide misconfiguration. Consider checking provider existence first, then throwing a clearer message like "is not compatible with" (optionally including allowed providers).
var setting = await SettingDefinitionManager.GetAsync(name);
if (setting.Providers.Any() && !setting.Providers.Contains(providerName))
{
throw new AbpException(
$"The setting named '{name}' has not compatible with the provider named '{providerName}'");
}
var providers = Enumerable
.Reverse(Providers)
.SkipWhile(p => p.Name != providerName)
.ToList();
if (!providers.Any())
{
throw new AbpException($"Unknown setting value provider: {providerName}");
}
modules/feature-management/src/Volo.Abp.FeatureManagement.Domain/Volo/Abp/FeatureManagement/FeatureManager.cs:163
- The new compatibility exception message is grammatically incorrect ("has not compatible") and is checked before verifying the provider exists. For a feature with restricted
AllowedProviders, callingSetAsyncwith an unregisteredproviderNamewill throw the compatibility exception instead of the existing "Unknown feature value provider" error, which is misleading. Consider checking provider existence first, then throwing a clearer compatibility message (e.g., "is not compatible with") when the provider is registered but not allowed.
var feature = await FeatureDefinitionManager.GetAsync(name);
if (feature.ValueType?.Validator.IsValid(value) == false)
{
throw new FeatureValueInvalidException(feature.DisplayName.Localize(StringLocalizerFactory));
}
if (feature.AllowedProviders.Any() && !feature.AllowedProviders.Contains(providerName))
{
throw new AbpException(
$"The feature named '{name}' has not compatible with the provider named '{providerName}'");
}
var providers = Enumerable
.Reverse(Providers)
.SkipWhile(p => p.Name != providerName)
.ToList();
if (!providers.Any())
{
throw new AbpException($"Unknown feature value provider: {providerName}");
}
- SettingManager.GetAllAsync / FeatureManager.GetAllWithProviderAsync: drop top-level continue and rely on the provider chain filter so allowed upstream providers can still be read via inheritance (e.g. GetAllForUserAsync now returns a Global-only setting) - FeatureAppService.GetAsync: switch includedFeatures to HashSet for O(1) parent lookup
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
modules/setting-management/src/Volo.Abp.SettingManagement.Domain/Volo/Abp/SettingManagement/SettingManager.cs:144
SetAsynccheckssetting.Providerscompatibility before verifyingproviderNameexists in the configured management providers. IfproviderNameis actually unknown and the setting has a restricted provider list, this will throw the “not compatible” exception instead of the existing “Unknown setting value provider” error. Consider performing the unknown-provider check first (or only running the compatibility check afterproviders.Any()is validated).
var setting = await SettingDefinitionManager.GetAsync(name);
if (setting.Providers.Any() && !setting.Providers.Contains(providerName))
{
throw new AbpException(
$"The setting named '{name}' has not compatible with the provider named '{providerName}'");
}
var providers = Enumerable
.Reverse(Providers)
.SkipWhile(p => p.Name != providerName)
.ToList();
if (!providers.Any())
{
throw new AbpException($"Unknown setting value provider: {providerName}");
}
modules/setting-management/src/Volo.Abp.SettingManagement.Domain/Volo/Abp/SettingManagement/SettingManager.cs:107
- In
GetAllAsync, the non-inherited branch callssettingProviderList[0].GetOrNullAsync(setting, providerKey)even when the first allowed provider is not the requestedproviderName(after filtering bysetting.Providers). This can pass a user/tenant key to a different provider type and read the wrong row. Pass the key only when the provider matches the requested provider (otherwisenull), similar to the inherited branch logic.
else
{
value = await settingProviderList[0].GetOrNullAsync(
setting,
providerKey
);
}
modules/feature-management/src/Volo.Abp.FeatureManagement.Domain/Volo/Abp/FeatureManagement/FeatureManager.cs:163
SetAsyncchecksfeature.AllowedProviderscompatibility before verifyingproviderNameexists in the configured management providers. IfproviderNameis unknown and the feature hasAllowedProvidersconfigured, this throws the “not compatible” exception instead of the existing “Unknown feature value provider” error. Consider validating provider existence first, then enforcing theAllowedProvidersrestriction.
var feature = await FeatureDefinitionManager.GetAsync(name);
if (feature.ValueType?.Validator.IsValid(value) == false)
{
throw new FeatureValueInvalidException(feature.DisplayName.Localize(StringLocalizerFactory));
}
if (feature.AllowedProviders.Any() && !feature.AllowedProviders.Contains(providerName))
{
throw new AbpException(
$"The feature named '{name}' has not compatible with the provider named '{providerName}'");
}
var providers = Enumerable
.Reverse(Providers)
.SkipWhile(p => p.Name != providerName)
.ToList();
if (!providers.Any())
{
throw new AbpException($"Unknown feature value provider: {providerName}");
}
Replace 'has not compatible with' with 'is not compatible with' in FeatureManager, SettingManager and PermissionManager (the latter is the original phrasing this PR initially mirrored).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
modules/setting-management/src/Volo.Abp.SettingManagement.Domain/Volo/Abp/SettingManagement/SettingManager.cs:107
- In GetAllAsync, for non-inherited settings (setting.IsInherited == false) the code reads from settingProviderList[0]. Since providerList is ordered from upstream -> current provider (e.g., DefaultValue/Configuration/.../User), this can ignore the requested provider and cause GetAllForUserAsync/GetAllGlobalAsync to miss values that actually exist for that provider. For non-inherited settings, this should only query the current providerName (and if the setting’s Providers list doesn’t include providerName, it should be skipped).
{
value = providerValue;
}
}
}
else
{
value = await settingProviderList[0].GetOrNullAsync(
setting,
providerKey
);
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rel-10.4 #25462 +/- ##
==========================================
Coverage 49.39% 49.40%
==========================================
Files 3670 3670
Lines 123594 123735 +141
Branches 9452 9463 +11
==========================================
+ Hits 61055 61127 +72
- Misses 60705 60780 +75
+ Partials 1834 1828 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixes https://abp.io/support/questions/10684. Feature/Setting management now respects
AllowedProviders/Providersto align withIFeatureChecker/ISettingProvider.