Merge pull request #505 from microsoft/rossgrambo/snapshot-variant-fix

rossgrambo · web-flow · commit f47e188babea · 2024-09-26T08:59:53.000-07:00
Resolves snapshot breaking change and adjusts feature tag helper as well
diff --git a/src/Microsoft.FeatureManagement.AspNetCore/TagHelpers/FeatureTagHelper.cs b/src/Microsoft.FeatureManagement.AspNetCore/TagHelpers/FeatureTagHelper.cs
@@ -14,7 +14,8 @@ namespace Microsoft.FeatureManagement.Mvc.TagHelpers
     /// </summary>
     public class FeatureTagHelper : TagHelper
     {
-        private readonly IVariantFeatureManager _featureManager;
+        private readonly IFeatureManager _featureManager;
+        private readonly IVariantFeatureManager _variantFeatureManager;
 
         /// <summary>
         /// A feature name, or comma separated list of feature names, for which the content should be rendered. By default, all specified features must be enabled to render the content, but this requirement can be controlled by the <see cref="Requirement"/> property.
@@ -41,9 +42,12 @@ public class FeatureTagHelper : TagHelper
         /// Creates a feature tag helper.
         /// </summary>
         /// <param name="featureManager">The feature manager snapshot to use to evaluate feature state.</param>
-        public FeatureTagHelper(IVariantFeatureManagerSnapshot featureManager)
+        /// <param name="variantFeatureManager">The variant feature manager snapshot to use to evaluate feature state.</param>
+        public FeatureTagHelper(IFeatureManagerSnapshot featureManager, IVariantFeatureManagerSnapshot variantFeatureManager)
         {
+            // Takes both a feature manager and a variant feature manager for backwards compatibility.
             _featureManager = featureManager ?? throw new ArgumentNullException(nameof(featureManager));
+            _variantFeatureManager = variantFeatureManager ?? throw new ArgumentNullException(nameof(variantFeatureManager));
         }
 
         /// <summary>
@@ -84,7 +88,7 @@ public override async Task ProcessAsync(TagHelperContext context, TagHelperOutpu
                     enabled = await variants.Any(
                         async variant =>
                         {
-                            Variant assignedVariant = await _featureManager.GetVariantAsync(features.First()).ConfigureAwait(false);
+                            Variant assignedVariant = await _variantFeatureManager.GetVariantAsync(features.First()).ConfigureAwait(false);
 
                             return variant == assignedVariant?.Name;
                         });
diff --git a/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs b/src/Microsoft.FeatureManagement/FeatureManagerSnapshot.cs
@@ -16,19 +16,37 @@ namespace Microsoft.FeatureManagement
     /// </summary>
     class FeatureManagerSnapshot : IFeatureManagerSnapshot, IVariantFeatureManagerSnapshot
     {
-        private readonly IVariantFeatureManager _featureManager;
+        private readonly IFeatureManager _featureManager;
+        private readonly IVariantFeatureManager _variantFeatureManager;
         private readonly ConcurrentDictionary<string, ValueTask<bool>> _flagCache = new ConcurrentDictionary<string, ValueTask<bool>>();
         private readonly ConcurrentDictionary<string, Variant> _variantCache = new ConcurrentDictionary<string, Variant>();
         private IEnumerable<string> _featureNames;
 
-        public FeatureManagerSnapshot(IVariantFeatureManager featureManager)
+        // Takes both a feature manager and a variant feature manager for backwards compatibility.
+        public FeatureManagerSnapshot(IFeatureManager featureManager, IVariantFeatureManager variantFeatureManager)
         {
             _featureManager = featureManager ?? throw new ArgumentNullException(nameof(featureManager));
+            _variantFeatureManager = variantFeatureManager ?? throw new ArgumentNullException(nameof(variantFeatureManager));
         }
 
-        public IAsyncEnumerable<string> GetFeatureNamesAsync()
+        public async IAsyncEnumerable<string> GetFeatureNamesAsync()
         {
-            return GetFeatureNamesAsync(CancellationToken.None);
+            if (_featureNames == null)
+            {
+                var featureNames = new List<string>();
+
+                await foreach (string featureName in _featureManager.GetFeatureNamesAsync().ConfigureAwait(false))
+                {
+                    featureNames.Add(featureName);
+                }
+
+                _featureNames = featureNames;
+            }
+
+            foreach (string featureName in _featureNames)
+            {
+                yield return featureName;
+            }
         }
 
         public async IAsyncEnumerable<string> GetFeatureNamesAsync([EnumeratorCancellation] CancellationToken cancellationToken)
@@ -37,7 +55,7 @@ public async IAsyncEnumerable<string> GetFeatureNamesAsync([EnumeratorCancellati
             {
                 var featureNames = new List<string>();
 
-                await foreach (string featureName in _featureManager.GetFeatureNamesAsync(cancellationToken).ConfigureAwait(false))
+                await foreach (string featureName in _variantFeatureManager.GetFeatureNamesAsync(cancellationToken).ConfigureAwait(false))
                 {
                     featureNames.Add(featureName);
                 }
@@ -55,28 +73,28 @@ public Task<bool> IsEnabledAsync(string feature)
         {
             return _flagCache.GetOrAdd(
                 feature,
-                (key) => _featureManager.IsEnabledAsync(key, CancellationToken.None)).AsTask();
+                (key) => new ValueTask<bool>(_featureManager.IsEnabledAsync(key))).AsTask();
         }
 
         public Task<bool> IsEnabledAsync<TContext>(string feature, TContext context)
         {
             return _flagCache.GetOrAdd(
                 feature,
-                (key) => _featureManager.IsEnabledAsync(key, context, CancellationToken.None)).AsTask();
+                (key) => new ValueTask<bool>(_featureManager.IsEnabledAsync(key, context))).AsTask();
         }
 
         public ValueTask<bool> IsEnabledAsync(string feature, CancellationToken cancellationToken)
         {
             return _flagCache.GetOrAdd(
                 feature,
-                (key) => _featureManager.IsEnabledAsync(key, cancellationToken));
+                (key) => _variantFeatureManager.IsEnabledAsync(key, cancellationToken));
         }
 
         public ValueTask<bool> IsEnabledAsync<TContext>(string feature, TContext context, CancellationToken cancellationToken)
         {
             return _flagCache.GetOrAdd(
                 feature,
-                (key) => _featureManager.IsEnabledAsync(key, context, cancellationToken));
+                (key) => _variantFeatureManager.IsEnabledAsync(key, context, cancellationToken));
         }
 
         public async ValueTask<Variant> GetVariantAsync(string feature, CancellationToken cancellationToken)
@@ -90,7 +108,7 @@ public async ValueTask<Variant> GetVariantAsync(string feature, CancellationToke
                 return _variantCache[cacheKey];
             }
 
-            Variant variant = await _featureManager.GetVariantAsync(feature, cancellationToken).ConfigureAwait(false);
+            Variant variant = await _variantFeatureManager.GetVariantAsync(feature, cancellationToken).ConfigureAwait(false);
 
             _variantCache[cacheKey] = variant;
 
@@ -108,7 +126,7 @@ public async ValueTask<Variant> GetVariantAsync(string feature, ITargetingContex
                 return _variantCache[cacheKey];
             }
 
-            Variant variant = await _featureManager.GetVariantAsync(feature, context, cancellationToken).ConfigureAwait(false);
+            Variant variant = await _variantFeatureManager.GetVariantAsync(feature, context, cancellationToken).ConfigureAwait(false);
 
             _variantCache[cacheKey] = variant;
 
diff --git a/src/Microsoft.FeatureManagement/ServiceCollectionExtensions.cs b/src/Microsoft.FeatureManagement/ServiceCollectionExtensions.cs
@@ -34,13 +34,7 @@ public static IFeatureManagementBuilder AddFeatureManagement(this IServiceCollec
                     "Scoped feature management has been registered.");
             }
 
-            services.AddLogging();
-
-            services.AddMemoryCache();
-
-            //
-            // Add required services
-            services.TryAddSingleton<IFeatureDefinitionProvider, ConfigurationFeatureDefinitionProvider>();
+            AddCommonFeatureManagementServices(services);
 
             services.AddSingleton(sp => new FeatureManager(
                             sp.GetRequiredService<IFeatureDefinitionProvider>(),
@@ -58,27 +52,7 @@ public static IFeatureManagementBuilder AddFeatureManagement(this IServiceCollec
 
             services.TryAddSingleton<IVariantFeatureManager>(sp => sp.GetRequiredService<FeatureManager>());
 
-            services.AddScoped<FeatureManagerSnapshot>();
-
-            services.TryAddScoped<IFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>());
-
-            services.TryAddScoped<IVariantFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>());
-
-            var builder = new FeatureManagementBuilder(services);
-
-            //
-            // Add built-in feature filters
-            builder.AddFeatureFilter<PercentageFilter>();
-
-            builder.AddFeatureFilter<TimeWindowFilter>(sp =>
-                new TimeWindowFilter()
-                {
-                    Cache = sp.GetRequiredService<IMemoryCache>()
-                });
-
-            builder.AddFeatureFilter<ContextualTargetingFilter>();
-
-            return builder;
+            return GetFeatureManagementBuilder(services);
         }
 
         /// <summary>
@@ -120,13 +94,7 @@ public static IFeatureManagementBuilder AddScopedFeatureManagement(this IService
                     "Singleton feature management has been registered.");
             }
 
-            services.AddLogging();
-
-            services.AddMemoryCache();
-
-            //
-            // Add required services
-            services.TryAddSingleton<IFeatureDefinitionProvider, ConfigurationFeatureDefinitionProvider>();
+            AddCommonFeatureManagementServices(services);
 
             services.AddScoped(sp => new FeatureManager(
                             sp.GetRequiredService<IFeatureDefinitionProvider>(),
@@ -144,27 +112,7 @@ public static IFeatureManagementBuilder AddScopedFeatureManagement(this IService
 
             services.TryAddScoped<IVariantFeatureManager>(sp => sp.GetRequiredService<FeatureManager>());
 
-            services.AddScoped<FeatureManagerSnapshot>();
-
-            services.TryAddScoped<IFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>());
-
-            services.TryAddScoped<IVariantFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>());
-
-            var builder = new FeatureManagementBuilder(services);
-
-            //
-            // Add built-in feature filters
-            builder.AddFeatureFilter<PercentageFilter>();
-
-            builder.AddFeatureFilter<TimeWindowFilter>(sp =>
-                new TimeWindowFilter()
-                {
-                    Cache = sp.GetRequiredService<IMemoryCache>()
-                });
-
-            builder.AddFeatureFilter<ContextualTargetingFilter>();
-
-            return builder;
+            return GetFeatureManagementBuilder(services);
         }
 
         /// <summary>
@@ -190,5 +138,39 @@ public static IFeatureManagementBuilder AddScopedFeatureManagement(this IService
 
             return services.AddScopedFeatureManagement();
         }
+
+        private static void AddCommonFeatureManagementServices(IServiceCollection services)
+        {
+            services.AddLogging();
+
+            services.AddMemoryCache();
+
+            services.TryAddSingleton<IFeatureDefinitionProvider, ConfigurationFeatureDefinitionProvider>();
+
+            services.AddScoped<FeatureManagerSnapshot>();
+
+            services.TryAddScoped<IFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>());
+
+            services.TryAddScoped<IVariantFeatureManagerSnapshot>(sp => sp.GetRequiredService<FeatureManagerSnapshot>());
+        }
+
+        private static IFeatureManagementBuilder GetFeatureManagementBuilder(IServiceCollection services)
+        {
+            var builder = new FeatureManagementBuilder(services);
+
+            //
+            // Add built-in feature filters
+            builder.AddFeatureFilter<PercentageFilter>();
+
+            builder.AddFeatureFilter<TimeWindowFilter>(sp =>
+                new TimeWindowFilter()
+                {
+                    Cache = sp.GetRequiredService<IMemoryCache>()
+                });
+
+            builder.AddFeatureFilter<ContextualTargetingFilter>();
+
+            return builder;
+        }
     }
 }
diff --git a/tests/Tests.FeatureManagement.AspNetCore/FeatureManagementAspNetCore.cs b/tests/Tests.FeatureManagement.AspNetCore/FeatureManagementAspNetCore.cs
@@ -5,10 +5,12 @@
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Razor.TagHelpers;
 using Microsoft.AspNetCore.TestHost;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.FeatureManagement;
+using Microsoft.FeatureManagement.Mvc.TagHelpers;
 using System.Collections.Generic;
 using System.Linq;
 using System.Net;
@@ -181,4 +183,71 @@ private static void DisableEndpointRouting(MvcOptions options)
             options.EnableEndpointRouting = false;
         }
     }
+
+    public class CustomImplementationsFeatureManagementTests
+    {
+        public class CustomIFeatureManager : IFeatureManager
+        {
+            public IAsyncEnumerable<string> GetFeatureNamesAsync()
+            {
+                return new string[1] { "Test" }.ToAsyncEnumerable();
+            }
+
+            public async Task<bool> IsEnabledAsync(string feature)
+            {
+                return await Task.FromResult(feature == "Test");
+            }
+
+            public async Task<bool> IsEnabledAsync<TContext>(string feature, TContext context)
+            {
+                return await Task.FromResult(feature == "Test");
+            }
+        }
+
+        [Fact]
+        public async Task CustomIFeatureManagerAspNetCoreTest()
+        {
+            IConfiguration config = new ConfigurationBuilder().AddJsonFile("appsettings.json").Build();
+
+            var services = new ServiceCollection();
+
+            services.AddSingleton(config)
+                    .AddSingleton<IFeatureManager, CustomIFeatureManager>()
+                    .AddFeatureManagement(); // Shouldn't override
+
+            ServiceProvider serviceProvider = services.BuildServiceProvider();
+
+            IFeatureManager featureManager = serviceProvider.GetRequiredService<IFeatureManager>();
+
+            Assert.True(await featureManager.IsEnabledAsync("Test"));
+            Assert.False(await featureManager.IsEnabledAsync("NotTest"));
+
+            // FeatureTagHelper should use available IFeatureManager
+            FeatureTagHelper featureTagHelper = new FeatureTagHelper(serviceProvider.GetRequiredService<IFeatureManagerSnapshot>(), serviceProvider.GetRequiredService<IVariantFeatureManagerSnapshot>());
+            TagHelperOutput tagHelperOutput = new TagHelperOutput("TestTag", new TagHelperAttributeList(), (aBool, aHtmlEncoder) => { return null; });
+
+            // Test returns true, so it shouldn't be modified
+            featureTagHelper.Name = "Test";
+            Assert.False(tagHelperOutput.IsContentModified);
+            await featureTagHelper.ProcessAsync(null, tagHelperOutput);
+            Assert.False(tagHelperOutput.IsContentModified);
+
+            tagHelperOutput.Reinitialize("TestTag", TagMode.StartTagAndEndTag);
+
+            // NotTest returns false, so it should be modified
+            featureTagHelper.Name = "NotTest";
+            Assert.False(tagHelperOutput.IsContentModified);
+            await featureTagHelper.ProcessAsync(null, tagHelperOutput);
+            Assert.True(tagHelperOutput.IsContentModified);
+
+            tagHelperOutput.Reinitialize("TestTag", TagMode.StartTagAndEndTag);
+
+            // When variant is used, Test flag should no longer exist and return false
+            featureTagHelper.Name = "Test";
+            featureTagHelper.Variant = "Something";
+            Assert.False(tagHelperOutput.IsContentModified);
+            await featureTagHelper.ProcessAsync(null, tagHelperOutput);
+            Assert.True(tagHelperOutput.IsContentModified);
+        }
+    }
 }
diff --git a/tests/Tests.FeatureManagement.AspNetCore/Tests.FeatureManagement.AspNetCore.csproj b/tests/Tests.FeatureManagement.AspNetCore/Tests.FeatureManagement.AspNetCore.csproj
@@ -15,6 +15,7 @@
 
   <ItemGroup>
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.11.0" />
+    <PackageReference Include="System.Linq.Async" Version="6.0.1" />
     <PackageReference Include="xunit" Version="2.9.0" />
     <PackageReference Include="xunit.runner.visualstudio" Version="2.8.2" />
   </ItemGroup>
diff --git a/tests/Tests.FeatureManagement/FeatureManagementTest.cs b/tests/Tests.FeatureManagement/FeatureManagementTest.cs
diff --git a/tests/Tests.FeatureManagement/Tests.FeatureManagement.csproj b/tests/Tests.FeatureManagement/Tests.FeatureManagement.csproj

Original file line number	Diff line number	Diff line change
`@@ -16,19 +16,37 @@ namespace Microsoft.FeatureManagement`
`16`	`16`	`/// </summary>`
`17`	`17`	`class FeatureManagerSnapshot : IFeatureManagerSnapshot, IVariantFeatureManagerSnapshot`
`18`	`18`	`{`
`19`		`- private readonly IVariantFeatureManager _featureManager;`
	`19`	`+ private readonly IFeatureManager _featureManager;`
	`20`	`+ private readonly IVariantFeatureManager _variantFeatureManager;`
`20`	`21`	`private readonly ConcurrentDictionary<string, ValueTask<bool>> _flagCache = new ConcurrentDictionary<string, ValueTask<bool>>();`
`21`	`22`	`private readonly ConcurrentDictionary<string, Variant> _variantCache = new ConcurrentDictionary<string, Variant>();`
`22`	`23`	`private IEnumerable<string> _featureNames;`
`23`	`24`
`24`		`- public FeatureManagerSnapshot(IVariantFeatureManager featureManager)`
	`25`	`+ // Takes both a feature manager and a variant feature manager for backwards compatibility.`
	`26`	`+ public FeatureManagerSnapshot(IFeatureManager featureManager, IVariantFeatureManager variantFeatureManager)`
`25`	`27`	`{`
`26`	`28`	`_featureManager = featureManager ?? throw new ArgumentNullException(nameof(featureManager));`
	`29`	`+ _variantFeatureManager = variantFeatureManager ?? throw new ArgumentNullException(nameof(variantFeatureManager));`
`27`	`30`	`}`
`28`	`31`
`29`		`- public IAsyncEnumerable<string> GetFeatureNamesAsync()`
	`32`	`+ public async IAsyncEnumerable<string> GetFeatureNamesAsync()`
`30`	`33`	`{`
`31`		`- return GetFeatureNamesAsync(CancellationToken.None);`
	`34`	`+ if (_featureNames == null)`
	`35`	`+ {`
	`36`	`+ var featureNames = new List<string>();`
	`37`	`+`
	`38`	`+ await foreach (string featureName in _featureManager.GetFeatureNamesAsync().ConfigureAwait(false))`
	`39`	`+ {`
	`40`	`+ featureNames.Add(featureName);`
	`41`	`+ }`
	`42`	`+`
	`43`	`+ _featureNames = featureNames;`
	`44`	`+ }`
	`45`	`+`
	`46`	`+ foreach (string featureName in _featureNames)`
	`47`	`+ {`
	`48`	`+ yield return featureName;`
	`49`	`+ }`
`32`	`50`	`}`
`33`	`51`
`34`	`52`	`public async IAsyncEnumerable<string> GetFeatureNamesAsync([EnumeratorCancellation] CancellationToken cancellationToken)`
`@@ -37,7 +55,7 @@ public async IAsyncEnumerable<string> GetFeatureNamesAsync([EnumeratorCancellati`
`37`	`55`	`{`
`38`	`56`	`var featureNames = new List<string>();`
`39`	`57`
`40`		`- await foreach (string featureName in _featureManager.GetFeatureNamesAsync(cancellationToken).ConfigureAwait(false))`
	`58`	`+ await foreach (string featureName in _variantFeatureManager.GetFeatureNamesAsync(cancellationToken).ConfigureAwait(false))`
`41`	`59`	`{`
`42`	`60`	`featureNames.Add(featureName);`
`43`	`61`	`}`
`@@ -55,28 +73,28 @@ public Task<bool> IsEnabledAsync(string feature)`
`55`	`73`	`{`
`56`	`74`	`return _flagCache.GetOrAdd(`
`57`	`75`	`feature,`
`58`		`- (key) => _featureManager.IsEnabledAsync(key, CancellationToken.None)).AsTask();`
	`76`	`+ (key) => new ValueTask<bool>(_featureManager.IsEnabledAsync(key))).AsTask();`
`59`	`77`	`}`
`60`	`78`
`61`	`79`	`public Task<bool> IsEnabledAsync<TContext>(string feature, TContext context)`
`62`	`80`	`{`
`63`	`81`	`return _flagCache.GetOrAdd(`
`64`	`82`	`feature,`
`65`		`- (key) => _featureManager.IsEnabledAsync(key, context, CancellationToken.None)).AsTask();`
	`83`	`+ (key) => new ValueTask<bool>(_featureManager.IsEnabledAsync(key, context))).AsTask();`
`66`	`84`	`}`
`67`	`85`
`68`	`86`	`public ValueTask<bool> IsEnabledAsync(string feature, CancellationToken cancellationToken)`
`69`	`87`	`{`
`70`	`88`	`return _flagCache.GetOrAdd(`
`71`	`89`	`feature,`
`72`		`- (key) => _featureManager.IsEnabledAsync(key, cancellationToken));`
	`90`	`+ (key) => _variantFeatureManager.IsEnabledAsync(key, cancellationToken));`
`73`	`91`	`}`
`74`	`92`
`75`	`93`	`public ValueTask<bool> IsEnabledAsync<TContext>(string feature, TContext context, CancellationToken cancellationToken)`
`76`	`94`	`{`
`77`	`95`	`return _flagCache.GetOrAdd(`
`78`	`96`	`feature,`
`79`		`- (key) => _featureManager.IsEnabledAsync(key, context, cancellationToken));`
	`97`	`+ (key) => _variantFeatureManager.IsEnabledAsync(key, context, cancellationToken));`
`80`	`98`	`}`
`81`	`99`
`82`	`100`	`public async ValueTask<Variant> GetVariantAsync(string feature, CancellationToken cancellationToken)`
`@@ -90,7 +108,7 @@ public async ValueTask<Variant> GetVariantAsync(string feature, CancellationToke`
`90`	`108`	`return _variantCache[cacheKey];`
`91`	`109`	`}`
`92`	`110`
`93`		`- Variant variant = await _featureManager.GetVariantAsync(feature, cancellationToken).ConfigureAwait(false);`
	`111`	`+ Variant variant = await _variantFeatureManager.GetVariantAsync(feature, cancellationToken).ConfigureAwait(false);`
`94`	`112`
`95`	`113`	`_variantCache[cacheKey] = variant;`
`96`	`114`
`@@ -108,7 +126,7 @@ public async ValueTask<Variant> GetVariantAsync(string feature, ITargetingContex`
`108`	`126`	`return _variantCache[cacheKey];`
`109`	`127`	`}`
`110`	`128`
`111`		`- Variant variant = await _featureManager.GetVariantAsync(feature, context, cancellationToken).ConfigureAwait(false);`
	`129`	`+ Variant variant = await _variantFeatureManager.GetVariantAsync(feature, context, cancellationToken).ConfigureAwait(false);`
`112`	`130`
`113`	`131`	`_variantCache[cacheKey] = variant;`
`114`	`132`