From 686f204e63692b0222e0a28cc7c0cafec0d41d04 Mon Sep 17 00:00:00 2001 From: Jimmy Campbell Date: Wed, 18 May 2022 10:09:13 -0700 Subject: [PATCH 1/2] Fixed targeting assignment precedence. --- ...ntextualTargetingFeatureVariantAssigner.cs | 119 +++++++++++++----- .../Targeting/ContextualTargetingFilter.cs | 2 +- .../Targeting/TargetingEvaluator.cs | 114 +++++++++++++++-- .../FeatureManagement.cs | 57 +++++++++ tests/Tests.FeatureManagement/Features.cs | 1 + .../Tests.FeatureManagement/appsettings.json | 46 +++++++ 6 files changed, 295 insertions(+), 44 deletions(-) diff --git a/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFeatureVariantAssigner.cs b/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFeatureVariantAssigner.cs index e703da76..6d1355f2 100644 --- a/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFeatureVariantAssigner.cs +++ b/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFeatureVariantAssigner.cs @@ -67,69 +67,128 @@ public ValueTask AssignVariantAsync(FeatureVariantAssignmentCont FeatureVariant variant = null; - double cumulativePercentage = 0; + var lookup = new Dictionary(); + + // + // Check users + foreach (FeatureVariant v in featureDefinition.Variants) + { + TargetingFilterSettings targetingSettings = v.AssignmentParameters.Get(); + + // + // Put in lookup table to avoid repeatedly creating targeting settings + lookup[v] = targetingSettings; + + if (targetingSettings == null && + v.Default) + { + // + // Valid to omit audience for default variant + continue; + } + + if (!TargetingEvaluator.TryValidateSettings(targetingSettings, out string paramName, out string reason)) + { + throw new ArgumentException(reason, paramName); + } + + // + // Check if the user is being targeted directly + if (targetingSettings.Audience.Users != null && + TargetingEvaluator.IsTargeted( + targetingContext, + targetingSettings.Audience.Users, + _options.IgnoreCase)) + { + return new ValueTask(v); + } + } var cumulativeGroups = new Dictionary( _options.IgnoreCase ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); + // + // Check Groups foreach (FeatureVariant v in featureDefinition.Variants) { - TargetingFilterSettings targetingSettings = v.AssignmentParameters.Get(); + TargetingFilterSettings targetingSettings = lookup[v]; - if (targetingSettings == null) + if (targetingSettings == null || + targetingSettings.Audience.Groups == null) { - if (v.Default) - { - // - // Valid to omit audience for default variant - continue; - } + continue; } - if (!TargetingEvaluator.TryValidateSettings(targetingSettings, out string paramName, out string reason)) + AccumulateGroups(targetingSettings.Audience, cumulativeGroups); + + if (TargetingEvaluator.IsTargeted( + targetingContext, + targetingSettings.Audience.Groups, + _options.IgnoreCase, + featureDefinition.Name)) { - throw new ArgumentException(reason, paramName); + return new ValueTask(v); } + } + + double cumulativePercentage = 0; - AccumulateAudience(targetingSettings.Audience, cumulativeGroups, ref cumulativePercentage); + // + // Check default rollout percentage + foreach (FeatureVariant v in featureDefinition.Variants) + { + TargetingFilterSettings targetingSettings = lookup[v]; - if (TargetingEvaluator.IsTargeted(targetingSettings, targetingContext, _options.IgnoreCase, featureDefinition.Name)) + if (targetingSettings == null) { - variant = v; + continue; + } + + AccumulateDefaultRollout(targetingSettings.Audience, ref cumulativePercentage); - break; + if (TargetingEvaluator.IsTargeted( + targetingContext, + targetingSettings.Audience.DefaultRolloutPercentage, + _options.IgnoreCase, + featureDefinition.Name)) + { + return new ValueTask(v); } } - return new ValueTask(variant); + return new ValueTask((FeatureVariant)null); } /// - /// Accumulates percentages for groups and the default rollout for an audience. + /// Accumulates percentages for groups of an audience. /// /// The audience that will have its percentages updated based on currently accumulated percentages - /// The current cumulative default rollout percentage /// The current cumulative rollout percentage for each group - private static void AccumulateAudience(Audience audience, Dictionary cumulativeGroups, ref double cumulativeDefaultPercentage) + private static void AccumulateGroups(Audience audience, Dictionary cumulativeGroups) { - if (audience.Groups != null) + foreach (GroupRollout gr in audience.Groups) { - foreach (GroupRollout gr in audience.Groups) - { - double percentage = gr.RolloutPercentage; + double percentage = gr.RolloutPercentage; - if (cumulativeGroups.TryGetValue(gr.Name, out double p)) - { - percentage += p; - } + if (cumulativeGroups.TryGetValue(gr.Name, out double p)) + { + percentage += p; + } - cumulativeGroups[gr.Name] = percentage; + cumulativeGroups[gr.Name] = percentage; - gr.RolloutPercentage = percentage; - } + gr.RolloutPercentage = percentage; } + } + /// + /// Accumulates percentages for the default rollout of an audience. + /// + /// The audience that will have its percentages updated based on currently accumulated percentages + /// The current cumulative default rollout percentage + private static void AccumulateDefaultRollout(Audience audience, ref double cumulativeDefaultPercentage) + { cumulativeDefaultPercentage = cumulativeDefaultPercentage + audience.DefaultRolloutPercentage; audience.DefaultRolloutPercentage = cumulativeDefaultPercentage; diff --git a/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFilter.cs b/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFilter.cs index a6e4ffd2..47f6b03e 100644 --- a/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFilter.cs +++ b/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFilter.cs @@ -50,7 +50,7 @@ public Task EvaluateAsync(FeatureFilterEvaluationContext context, ITargeti TargetingFilterSettings settings = context.Parameters.Get() ?? new TargetingFilterSettings(); - return Task.FromResult(TargetingEvaluator.IsTargeted(settings, targetingContext, _options.IgnoreCase, context.FeatureName)); + return Task.FromResult(TargetingEvaluator.IsTargeted(targetingContext, settings, _options.IgnoreCase, context.FeatureName)); } } } diff --git a/src/Microsoft.FeatureManagement/Targeting/TargetingEvaluator.cs b/src/Microsoft.FeatureManagement/Targeting/TargetingEvaluator.cs index 3e33b38b..08b7b685 100644 --- a/src/Microsoft.FeatureManagement/Targeting/TargetingEvaluator.cs +++ b/src/Microsoft.FeatureManagement/Targeting/TargetingEvaluator.cs @@ -17,7 +17,10 @@ private static StringComparison GetComparisonType(bool ignoreCase) => StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal; - public static bool IsTargeted(TargetingFilterSettings settings, ITargetingContext targetingContext, bool ignoreCase, string hint) + /// + /// Checks if a provided targeting context should be targeted given targeting settings. + /// + public static bool IsTargeted(ITargetingContext targetingContext, TargetingFilterSettings settings, bool ignoreCase, string hint) { if (settings == null) { @@ -36,29 +39,95 @@ public static bool IsTargeted(TargetingFilterSettings settings, ITargetingContex // // Check if the user is being targeted directly + if (settings.Audience.Users != null && + IsTargeted( + targetingContext, + settings.Audience.Users, + ignoreCase)) + { + return true; + } + + // + // Check if the user is in a group that is being targeted + if (settings.Audience.Groups != null && + IsTargeted( + targetingContext, + settings.Audience.Groups, + ignoreCase, + hint)) + { + return true; + } + + // + // Check if the user is being targeted by a default rollout percentage + return IsTargeted( + targetingContext, + settings.Audience.DefaultRolloutPercentage, + ignoreCase, + hint); + } + + /// + /// Determines if a targeting context is targeted by presence in a list of users + /// + public static bool IsTargeted( + ITargetingContext targetingContext, + IEnumerable users, + bool ignoreCase) + { + if (targetingContext == null) + { + throw new ArgumentNullException(nameof(targetingContext)); + } + + if (users == null) + { + throw new ArgumentNullException(nameof(users)); + } + if (targetingContext.UserId != null && - settings.Audience.Users != null && - settings.Audience.Users.Any(user => targetingContext.UserId.Equals(user, GetComparisonType(ignoreCase)))) + users.Any(user => targetingContext.UserId.Equals(user, GetComparisonType(ignoreCase)))) { return true; } + return false; + } + + /// + /// Determine if a targeting context is targeted by presence in a group + /// + public static bool IsTargeted( + ITargetingContext targetingContext, + IEnumerable groups, + bool ignoreCase, + string hint) + { + if (targetingContext == null) + { + throw new ArgumentNullException(nameof(targetingContext)); + } + + if (groups == null) + { + throw new ArgumentNullException(nameof(groups)); + } + string userId = ignoreCase ? targetingContext.UserId.ToLower() : targetingContext.UserId; - // - // Check if the user is in a group that is being targeted - if (targetingContext.Groups != null && - settings.Audience.Groups != null) + if (targetingContext.Groups != null) { - IEnumerable groups = ignoreCase ? + IEnumerable normalizedGroups = ignoreCase ? targetingContext.Groups.Select(g => g.ToLower()) : targetingContext.Groups; - foreach (string group in groups) + foreach (string group in normalizedGroups) { - GroupRollout groupRollout = settings.Audience.Groups.FirstOrDefault(g => g.Name.Equals(group, GetComparisonType(ignoreCase))); + GroupRollout groupRollout = groups.FirstOrDefault(g => g.Name.Equals(group, GetComparisonType(ignoreCase))); if (groupRollout != null) { @@ -72,11 +141,30 @@ public static bool IsTargeted(TargetingFilterSettings settings, ITargetingContex } } - // - // Check if the user is being targeted by a default rollout percentage + return false; + } + + /// + /// Determines if a targeting context is targeted by presence in a default rollout percentage. + /// + public static bool IsTargeted( + ITargetingContext targetingContext, + double defaultRolloutPercentage, + bool ignoreCase, + string hint) + { + if (targetingContext == null) + { + throw new ArgumentNullException(nameof(targetingContext)); + } + + string userId = ignoreCase ? + targetingContext.UserId.ToLower() : + targetingContext.UserId; + string defaultContextId = $"{userId}\n{hint}"; - return IsTargeted(defaultContextId, settings.Audience.DefaultRolloutPercentage); + return IsTargeted(defaultContextId, defaultRolloutPercentage); } /// diff --git a/tests/Tests.FeatureManagement/FeatureManagement.cs b/tests/Tests.FeatureManagement/FeatureManagement.cs index 4a935744..b84a970f 100644 --- a/tests/Tests.FeatureManagement/FeatureManagement.cs +++ b/tests/Tests.FeatureManagement/FeatureManagement.cs @@ -486,6 +486,63 @@ public async Task VariantTargeting() CancellationToken.None)); } + [Fact] + public async Task TargetingAssignmentPrecedence() + { + IConfiguration config = new ConfigurationBuilder().AddJsonFile("appsettings.json").Build(); + + var services = new ServiceCollection(); + + services + .Configure(options => + { + options.IgnoreMissingFeatureFilters = true; + }); + + services + .AddSingleton(config) + .AddFeatureManagement() + .AddFeatureVariantAssigner(); + + ServiceProvider serviceProvider = services.BuildServiceProvider(); + + IDynamicFeatureManager variantManager = serviceProvider.GetRequiredService(); + + // + // Assigned variant by default rollout due to no higher precedence match + Assert.Equal("def", await variantManager.GetVariantAsync( + Features.PrecedenceTestingFeature, + new TargetingContext + { + UserId = "Patty" + }, + CancellationToken.None)); + + // + // Assigned variant by group due to higher precedence than default rollout + Assert.Equal("ghi", await variantManager.GetVariantAsync( + Features.PrecedenceTestingFeature, + new TargetingContext + { + UserId = "Patty", + Groups = new string[] + { + "Ring0" + } + }, + CancellationToken.None)); + + // + // Assigned variant by user name to higher precedence than default rollout, and group match + Assert.Equal("jkl", await variantManager.GetVariantAsync( + Features.PrecedenceTestingFeature, + new TargetingContext + { + UserId = "Jeff" + }, + CancellationToken.None)); + } + [Fact] public async Task AccumulatesAudience() { diff --git a/tests/Tests.FeatureManagement/Features.cs b/tests/Tests.FeatureManagement/Features.cs index 3e8d791d..be136e16 100644 --- a/tests/Tests.FeatureManagement/Features.cs +++ b/tests/Tests.FeatureManagement/Features.cs @@ -13,5 +13,6 @@ static class Features public const string VariantFeature = "VariantFeature"; public const string ContextualVariantFeature = "ContextualVariantFeature"; public const string ContextualVariantTargetingFeature = "ContextualVariantTargetingFeature"; + public const string PrecedenceTestingFeature = "PrecedenceTestingFeature"; } } diff --git a/tests/Tests.FeatureManagement/appsettings.json b/tests/Tests.FeatureManagement/appsettings.json index 8ca72556..b26dc6a4 100644 --- a/tests/Tests.FeatureManagement/appsettings.json +++ b/tests/Tests.FeatureManagement/appsettings.json @@ -202,11 +202,57 @@ } } ] + }, + "PrecedenceTestingFeature": { + "Assigner": "Targeting", + "Variants": [ + { + "Default": true, + "Name": "V1", + "ConfigurationReference": "Ref1" + }, + { + "Name": "V2", + "ConfigurationReference": "Ref2", + "AssignmentParameters": { + "Audience": { + "DefaultRolloutPercentage": 100 + } + } + }, + { + "Name": "V3", + "ConfigurationReference": "Ref3", + "AssignmentParameters": { + "Audience": { + "Groups": [ + { + "Name": "Ring0", + "RolloutPercentage": 100 + } + ] + } + } + }, + { + "Name": "V4", + "ConfigurationReference": "Ref4", + "AssignmentParameters": { + "Audience": { + "Users": [ + "Jeff" + ] + } + } + } + ] } } }, "Ref1": "abc", "Ref2": "def", + "Ref3": "ghi", + "Ref4": "jkl", "Percentage15": 15, "Percentage35": 35, "Percentage50": 50 From d4cc815023c9ee2c0508688a44b9e90bdafb138c Mon Sep 17 00:00:00 2001 From: Jimmy Campbell Date: Tue, 24 May 2022 16:41:35 -0700 Subject: [PATCH 2/2] Remove unused variable. Update method signature. Added missing declaration in test. --- .../ContextualTargetingFeatureVariantAssigner.cs | 10 ++++------ tests/Tests.FeatureManagement/FeatureManagement.cs | 6 +++++- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFeatureVariantAssigner.cs b/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFeatureVariantAssigner.cs index 17af5506..d2de2e10 100644 --- a/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFeatureVariantAssigner.cs +++ b/src/Microsoft.FeatureManagement/Targeting/ContextualTargetingFeatureVariantAssigner.cs @@ -65,8 +65,6 @@ public ValueTask AssignVariantAsync(FeatureVariantAssignmentCont nameof(variantAssignmentContext)); } - FeatureVariant variant = null; - var lookup = new Dictionary(); // @@ -120,7 +118,7 @@ public ValueTask AssignVariantAsync(FeatureVariantAssignmentCont continue; } - AccumulateGroups(targetingSettings.Audience, cumulativeGroups); + AccumulateGroups(targetingSettings.Audience.Groups, cumulativeGroups); if (TargetingEvaluator.IsTargeted( targetingContext, @@ -163,11 +161,11 @@ public ValueTask AssignVariantAsync(FeatureVariantAssignmentCont /// /// Accumulates percentages for groups of an audience. /// - /// The audience that will have its percentages updated based on currently accumulated percentages + /// The groups that will have their percentages updated based on currently accumulated percentages /// The current cumulative rollout percentage for each group - private static void AccumulateGroups(Audience audience, Dictionary cumulativeGroups) + private static void AccumulateGroups(IEnumerable groups, Dictionary cumulativeGroups) { - foreach (GroupRollout gr in audience.Groups) + foreach (GroupRollout gr in groups) { double percentage = gr.RolloutPercentage; diff --git a/tests/Tests.FeatureManagement/FeatureManagement.cs b/tests/Tests.FeatureManagement/FeatureManagement.cs index 18cb0ce4..f06fc789 100644 --- a/tests/Tests.FeatureManagement/FeatureManagement.cs +++ b/tests/Tests.FeatureManagement/FeatureManagement.cs @@ -538,7 +538,11 @@ public async Task TargetingAssignmentPrecedence() Features.PrecedenceTestingFeature, new TargetingContext { - UserId = "Jeff" + UserId = "Jeff", + Groups = new string[] + { + "Ring0" + } }, CancellationToken.None)); }