diff --git a/src/Microsoft.FeatureManagement/FeatureFilterEvaluationContext.cs b/src/Microsoft.FeatureManagement/FeatureFilterEvaluationContext.cs index ccc7a884..0ea5f730 100644 --- a/src/Microsoft.FeatureManagement/FeatureFilterEvaluationContext.cs +++ b/src/Microsoft.FeatureManagement/FeatureFilterEvaluationContext.cs @@ -22,13 +22,12 @@ public class FeatureFilterEvaluationContext public IConfiguration Parameters { get; set; } /// - /// The settings provided for the feature filter to use when evaluating whether the feature should be enabled. This property takes precedence over and if both are provided. - /// - public object ParametersObject { get; set; } - - /// - /// A settings object, if any, that has been pre-bound from . - /// The settings are made available for s that implement . + /// A settings object, if any, provided for the feature filter to use when evaluating whether the feature should be enabled. + /// This property is populated in two cases: + /// + /// For features that provide parameters as an object, via . + /// For s that implement . + /// /// public object Settings { get; set; } diff --git a/src/Microsoft.FeatureManagement/FeatureFilters/PercentageFilter.cs b/src/Microsoft.FeatureManagement/FeatureFilters/PercentageFilter.cs index 044215f8..91bf6c44 100644 --- a/src/Microsoft.FeatureManagement/FeatureFilters/PercentageFilter.cs +++ b/src/Microsoft.FeatureManagement/FeatureFilters/PercentageFilter.cs @@ -49,18 +49,16 @@ public Task EvaluateAsync(FeatureFilterEvaluationContext context) throw new ArgumentNullException(nameof(context)); } - // - // Check if ParametersObject available (takes precedence), then prebound settings, otherwise bind from parameters. - PercentageFilterSettings settings; - - if (context.ParametersObject != null && !(context.ParametersObject is PercentageFilterSettings)) + if (context.Settings != null && !(context.Settings is PercentageFilterSettings)) { throw new ArgumentException( - $"The '{Alias}' feature filter for feature '{context.FeatureName}' has a {nameof(context.ParametersObject)} of type '{context.ParametersObject.GetType()}', but expected '{typeof(PercentageFilterSettings)}'.", - nameof(context.ParametersObject)); + $"The '{Alias}' feature filter for feature '{context.FeatureName}' has a {nameof(context.Settings)} value of type '{context.Settings.GetType()}', but expected '{typeof(PercentageFilterSettings)}'.", + nameof(context.Settings)); } - settings = (PercentageFilterSettings)context.ParametersObject ?? (PercentageFilterSettings)context.Settings ?? (PercentageFilterSettings)BindParameters(context.Parameters); + // + // Check if prebound settings available, otherwise bind from parameters. + PercentageFilterSettings settings = (PercentageFilterSettings)context.Settings ?? (PercentageFilterSettings)BindParameters(context.Parameters); bool result = true; diff --git a/src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs b/src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs index 608e8a1a..68cd3285 100644 --- a/src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs +++ b/src/Microsoft.FeatureManagement/FeatureFilters/TimeWindowFilter.cs @@ -70,18 +70,16 @@ public Task EvaluateAsync(FeatureFilterEvaluationContext context) throw new ArgumentNullException(nameof(context)); } - // - // Check if ParametersObject available (takes precedence), then prebound settings, otherwise bind from parameters. - TimeWindowFilterSettings settings; - - if (context.ParametersObject != null && !(context.ParametersObject is TimeWindowFilterSettings)) + if (context.Settings != null && !(context.Settings is TimeWindowFilterSettings)) { throw new ArgumentException( - $"The '{Alias}' feature filter for feature '{context.FeatureName}' has a {nameof(context.ParametersObject)} of type '{context.ParametersObject.GetType()}', but expected '{typeof(TimeWindowFilterSettings)}'.", - nameof(context.ParametersObject)); + $"The '{Alias}' feature filter for feature '{context.FeatureName}' has a {nameof(context.Settings)} value of type '{context.Settings.GetType()}', but expected '{typeof(TimeWindowFilterSettings)}'.", + nameof(context.Settings)); } - settings = (TimeWindowFilterSettings)context.ParametersObject ?? (TimeWindowFilterSettings)context.Settings ?? (TimeWindowFilterSettings)BindParameters(context.Parameters); + // + // Check if prebound settings available, otherwise bind from parameters. + TimeWindowFilterSettings settings = (TimeWindowFilterSettings)context.Settings ?? (TimeWindowFilterSettings)BindParameters(context.Parameters); DateTimeOffset now = SystemClock?.GetUtcNow() ?? DateTimeOffset.UtcNow; diff --git a/src/Microsoft.FeatureManagement/FeatureManager.cs b/src/Microsoft.FeatureManagement/FeatureManager.cs index c8c8eaa8..91605ecb 100644 --- a/src/Microsoft.FeatureManagement/FeatureManager.cs +++ b/src/Microsoft.FeatureManagement/FeatureManager.cs @@ -498,11 +498,14 @@ private async ValueTask IsEnabledAsync(FeatureDefinition feature { FeatureName = featureDefinition.Name, Parameters = featureFilterConfiguration.Parameters, - ParametersObject = featureFilterConfiguration.ParametersObject, + Settings = featureFilterConfiguration.ParametersObject, CancellationToken = cancellationToken }; - BindSettings(filter, context, filterIndex); + if (context.Settings == null) + { + BindSettings(filter, context, filterIndex); + } // // IContextualFeatureFilter @@ -681,6 +684,12 @@ private void BindSettings(IFeatureFilterMetadata filter, FeatureFilterEvaluation return; } + // Skip parameter binding if the provider has already supplied a parameters object. + if (context.Settings != null) + { + return; + } + if (!(context.Parameters is ConfigurationWrapper) || Cache == null) { context.Settings = binder.BindParameters(context.Parameters); diff --git a/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFilter.cs b/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFilter.cs index 014aa2af..bc592a3f 100644 --- a/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFilter.cs +++ b/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFilter.cs @@ -67,18 +67,16 @@ public Task EvaluateAsync(FeatureFilterEvaluationContext context, ITargeti throw new ArgumentNullException(nameof(targetingContext)); } - // - // Check if ParametersObject available (takes precedence), then prebound settings, otherwise bind from parameters. - TargetingFilterSettings settings; - - if (context.ParametersObject != null && !(context.ParametersObject is TargetingFilterSettings)) + if (context.Settings != null && !(context.Settings is TargetingFilterSettings)) { throw new ArgumentException( - $"The '{Alias}' feature filter for feature '{context.FeatureName}' has a {nameof(context.ParametersObject)} of type '{context.ParametersObject.GetType()}', but expected '{typeof(TargetingFilterSettings)}'.", - nameof(context.ParametersObject)); + $"The '{Alias}' feature filter for feature '{context.FeatureName}' has a {nameof(context.Settings)} value of type '{context.Settings.GetType()}', but expected '{typeof(TargetingFilterSettings)}'.", + nameof(context.Settings)); } - settings = (TargetingFilterSettings)context.ParametersObject ?? (TargetingFilterSettings)context.Settings ?? (TargetingFilterSettings)BindParameters(context.Parameters); + // + // Check if prebound settings available, otherwise bind from parameters. + TargetingFilterSettings settings = (TargetingFilterSettings)context.Settings ?? (TargetingFilterSettings)BindParameters(context.Parameters); return Task.FromResult(TargetingEvaluator.IsTargeted(targetingContext, settings, _options.IgnoreCase, context.FeatureName)); } diff --git a/tests/Tests.FeatureManagement/FeatureManagementTest.cs b/tests/Tests.FeatureManagement/FeatureManagementTest.cs index d38d7b5d..a70e6a0d 100644 --- a/tests/Tests.FeatureManagement/FeatureManagementTest.cs +++ b/tests/Tests.FeatureManagement/FeatureManagementTest.cs @@ -1094,9 +1094,9 @@ public async Task UsesParametersObject() testFeatureFilter.Callback = (evaluationContext) => { // - // When ParametersObject is set, it should be available on the context - // so custom filters can use it with their own precedence logic. - Assert.Same(parameterObject, evaluationContext.ParametersObject); + // When ParametersObject is set, it should be available on the context via settings + // so custom filters can use it. + Assert.Same(parameterObject, evaluationContext.Settings); return Task.FromResult(true); }; @@ -1157,7 +1157,6 @@ public async Task ParametersObjectFallsBackToParametersWhenNull() // // When ParametersObject is null, Settings should be populated // by IFilterParametersBinder as usual. - Assert.Null(evaluationContext.ParametersObject); Assert.NotNull(evaluationContext.Settings); return Task.FromResult(true); @@ -1925,6 +1924,121 @@ public async Task TimeWindowFilterThrowsOnInvalidParametersObjectType() await Assert.ThrowsAsync(() => featureManager.IsEnabledAsync("BadFeature")); } + + [Fact] + public async Task TargetingFilterUsesParametersObject() + { + var services = new ServiceCollection(); + + var definitionProvider = new InMemoryFeatureDefinitionProvider( + new FeatureDefinition[] + { + new FeatureDefinition + { + Name = "TargetingFeature", + EnabledFor = new List() + { + new FeatureFilterConfiguration + { + Name = "Microsoft.Targeting", + ParametersObject = new TargetingFilterSettings + { + Audience = new Audience + { + Users = new List { "Jeff" }, + Groups = new List + { + new GroupRollout + { + Name = "Ring1", + RolloutPercentage = 100 + } + }, + DefaultRolloutPercentage = 0 + } + } + } + } + } + }); + + services.AddSingleton(definitionProvider) + .AddSingleton(new ConfigurationBuilder().Build()) + .AddFeatureManagement(); + + ServiceProvider serviceProvider = services.BuildServiceProvider(); + + IVariantFeatureManager featureManager = serviceProvider.GetRequiredService(); + + // + // Targeted user should be enabled + var targetingContext = new TargetingContext + { + UserId = "Jeff", + Groups = new List { "Ring0" } + }; + + Assert.True(await featureManager.IsEnabledAsync("TargetingFeature", targetingContext)); + + // + // User in targeted group should be enabled + targetingContext = new TargetingContext + { + UserId = "NotTargeted", + Groups = new List { "Ring1" } + }; + + Assert.True(await featureManager.IsEnabledAsync("TargetingFeature", targetingContext)); + + // + // Non-targeted user should be disabled (0% default rollout) + targetingContext = new TargetingContext + { + UserId = "NotTargeted", + Groups = new List { "Ring0" } + }; + + Assert.False(await featureManager.IsEnabledAsync("TargetingFeature", targetingContext)); + } + + [Fact] + public async Task TargetingFilterThrowsOnInvalidParametersObjectType() + { + var services = new ServiceCollection(); + + var definitionProvider = new InMemoryFeatureDefinitionProvider( + new FeatureDefinition[] + { + new FeatureDefinition + { + Name = "BadFeature", + EnabledFor = new List() + { + new FeatureFilterConfiguration + { + Name = "Microsoft.Targeting", + ParametersObject = "wrong type" + } + } + } + }); + + services.AddSingleton(definitionProvider) + .AddSingleton(new ConfigurationBuilder().Build()) + .AddFeatureManagement(); + + ServiceProvider serviceProvider = services.BuildServiceProvider(); + + IVariantFeatureManager featureManager = serviceProvider.GetRequiredService(); + + var targetingContext = new TargetingContext + { + UserId = "Jeff", + Groups = new List() + }; + + await Assert.ThrowsAsync(() => featureManager.IsEnabledAsync("BadFeature", targetingContext).AsTask()); + } } public class FeatureManagementVariantTest