Feature Manager DI Scoping

[samgibsonmoj]

I have a net7 Blazor Server application and have integrated the Feature Management package into the application, registering various filters.

One filter, the 'TenantFilter' is responsible for enabling/disabling various areas of the system based on current users tenancy. This filter makes use of my 'ICurrentUserService', which is a scoped service that gives contextual information based on the user (e.g. tenant id, user id, user name).

I've noted that the Feature Manager (and other dependencies) are registered as a Singleton... There's no way around this scoping, as these types aren't made available outside of the library.

I'm unable to get any of the users contextual information via the HttpContextAccessor, as the HttpContext isn't guaranteed to be available via this interface.

While I understand there may be no enhancements planned for Blazor Server, I was curious on your take on this, and any potential solutions/workarounds. Currently, I've had to fork this repository, and register the services as Scoped.

Thanks.

[rossgrambo]

Hello @samgibsonmoj

It sounds like a workaround is:

The recommended approach for passing request state to the Blazor app is through root component parameters during the app's initial rendering.

I'm curious if you're able to pass this state when you construct your HTTPContextAccessor (create a new Accessor that accepts it as a parameter) so it could instead use that state instead of the normal HttpContext. Might not make sense with the lifecycle of Blazor apps, as I haven't used Blazor yet.

Otherwise I think your fork is the correct solution for now. I'll check with others about adding an injection option for Blazor.

[samgibsonmoj]

Thanks @rossgrambo, I appreciate the fast response.

I'm curious if you're able to pass this state when you construct your HTTPContextAccessor (create a new Accessor that accepts it as a parameter) so it could instead use that state instead of the normal HttpContext. Might not make sense with the lifecycle of Blazor apps, as I haven't used Blazor yet.

I'm not 100% sure this would work for what I'm looking to achieve. From what I can see, passing these values via the root component would make them available to other child components, but not within the Filter itself. Please let me know if I have misunderstood.

For some further clarification, the filter looks like this:

// TenantFilter.cs

[FilterAlias("App.Tenant")]
public class TenantFilter : IFeatureFilter
{
	private readonly ICurrentUserService _userService;

	public TenantFilter(ICurrentUserService userService)
	{
		this._userService = userService;
	}

	public Task<bool> EvaluateAsync(FeatureFilterEvaluationContext filterContext)
	{
		var settings = filterContext.Parameters.Get<TenantFilterSettings>() ?? throw new ArgumentNullException(nameof(TenantFilterSettings));
		var enabled = settings.Tenants.Any(t => t.Id == this._userService.TenantId);
		return Task.FromResult(enabled);
	}
}

I understand you can use an IContextualFeatureFilter, but without going into too much implementation detail, this filter is being used as part of a custom authorization attribute I have used to prevent access to various Blazor pages based on whether the feature is active. Similarly to how the FeatureGate attribute works with MVC, but for Blazor.

Otherwise I think your fork is the correct solution for now. I'll check with others about adding an injection option for Blazor.

I'll likely continue with this fork for now, and see what you and the others come back with. Even exposing these types as part of the public api would probably suffice. From there, we can pick and choose how we'd like to use/register them in our applications.

[rossgrambo]

I would think a solution here would be to define an accessor for your UserService like:

public class UserContextAccessor: ITargetingContextAccessor
{
    private readonly UserService _userService;

    public UserContextAccessor(UserService userService)
    {
        this._userService = userService;
    }

    public ValueTask<TargetingContext> GetContextAsync()
    {
        TargetingContext targetingContext = new TargetingContext
        {
            UserId = "" + _userService.getTenantId("abc"),
            Groups = new List<string>()
        };

        return new ValueTask<TargetingContext>(targetingContext);
    }
}

Once you register that, a call to TargetingFilter.IsEnabled() would use that context. Or if you wanted to stick with a custom filter, you could grab the accessor yourself

public CustomFilter(ITargetingContextAccessor contextAccessor) {

...

TargetingContext targetingContext = await _contextAccessor.GetContextAsync().ConfigureAwait(false);

The accessor could also use httpcontext to determine what the resulting targetingcontext should be.

Let me know if that makes sense, I may be misunderstanding.

[samgibsonmoj]

Thanks for coming back to me @rossgrambo.

Unfortunately, using an accessor doesn't avoid the issue I'm seeing here either. To clarify, the UserService is a Scoped dependency, and therefore cannot be consumed by another service with a longer lifetime than itself.

While it may then make logical sense to therefore register the UserContextAccessor as a Scoped dependency, this isn't possible as the (Singleton) Filter has a dependency on a Scoped Context Accessor:

Cannot consume scoped service 'Microsoft.FeatureManagement.FeatureFilters.ITargetingContextAccessor' from singleton 'Microsoft.FeatureManagement.IFeatureFilterMetadata'

This circles back to how the feature manager chooses to register its dependencies (as singletons).

Hope that makes more sense.

[zhiyuanliang-ms]

@samgibsonmoj Thank you for the explanation. I finally understand the issue #15.
I need to investigate this before I can give any advice here.

But I'm very willing to hear about what changes do you think we can make to solve your problem?
You've mentioned that

Even exposing these types as part of the public api would probably suffice.
What thing do you want to be public?

Could you provide a simple demo (maybe a public repo) to illustrate the problem better?

[samgibsonmoj]

Hi @zhiyuanliang-ms. Glad that cleared some things up.

What thing do you want to be public?

Please see ServiceCollectionExtensions.cs, where the Feature Manager DI extension methods are exposed. Exposing these dependencies (ConfigurationFeatureDefinitionProvider, FeatureManager, EmptySessionManager, FeatureManagerSnapshot) as part of the public api.

From here, we can pick and choose how we would like to register these dependencies, e.g:

// Program.cs

WebApplicationBuilder builder = WebApplication.CreateBuilder(args);

// Register Feature Management
builder.Services.AddScoped<IFeatureManager, FeatureManager>();
builder.Services.AddScoped<ISessionManager, EmptySessionManager>();

// Register Filters
builder.Services.AddScoped<IFeatureFilter, PercentageFilter>();
builder.Services.AddScoped<IFeatureFilter, CustomFilter>();

I hope that makes a little bit more sense. If necessary, I can construct a demonstration repository. However, this is more of an issue with the way dependency injection has been implemented, as opposed to a fundamental issue with any feature management functionality.

[samgibsonmoj]

These classes are the public interface into feature flags they shouldn't be internal, we need to instantiate them to use this.

This has also been requested here: #126

[samgibsonmoj]

See #3 from my fork for the proposed changes.

[samgibsonmoj]

Continuing discussions here for visibility. Comment below was left by @zhiyuanliang-ms on my Pull Request.

Hi, So far, we believe the HttpContextAccessor should be the right way to make the scoped HttpContext available to the singleton service. I understand that HttpContextAccessor should be avoided in Blazor app. I am investigating the workaround solution for Blazor app. Sorry for the inconvenience.

Thank you for taking a look, I'm interested to see what you come back with. Is there any reason you have decided against making these types available as part of the public api? In general, it allows the developer more flexibility in how each of the services should be registered, instead of forcing them to register everything as singletons... Imposing this global state is generally bad practice and seems like a misuse of DI in this case.

[zhiyuanliang-ms]

@samgibsonmoj Hi, we are considering provide an option to register feature manager and feature filter as scoped services. Currently, we do not want to expose feature manager class to public.

[McDoit]

@samgibsonmoj Hi, we are considering provide an option to register feature manager and feature filter as scoped services. Currently, we do not want to expose feature manager class to public.

Why not? What is gained by gatekeeping the constructors?

[jimmyca15]

@McDoit

Current thinking is not so much about gatekeeping but moreso to keep the ease of services.AddFeatureManagement(), which adds multiple things.

Something like services.AddScopedFeatureManagement(); And then we can expect that FeatureManager is a scoped service and can depend on scoped services.

[McDoit]

@jimmyca15 I'm a bit confused, can't you have public constructors while having a services.AddFeatureManagement() call for ease of service?
If you don't have any use of the public constructors you just use the standard servicecollection call, but we that have a usage of the public constructors can create our own installers etc?

[samgibsonmoj]

Something like services.AddScopedFeatureManagement(); And then we can expect that FeatureManager is a scoped service and can depend on scoped services.

Thanks for the response @jimmyca15. As @McDoit has correctly stated above, having the public constructor(s) wouldn't have any effect on the current AddFeatureManagement() extension method. This method would remain unaffected, and available for those who wish to continue to use it.

I can appreciate the thought behind adding an alternative AddScopedFeatureManagement() extension method, but transient services will then also have to be considered... To me, the simplest and best solution is to expose public constructors, allowing us to register these types as necessary. I don't foresee any further changes required outside of this.

[zhiyuanliang-ms]

@samgibsonmoj We are discussing how to expose the FeatureManager class. We are considering how to simplify the constructor of FeatureManager. The required parameters will be only IFeatureDefinitionProvider and FeatureManagmentOptions.

[McDoit]

@zhiyuanliang-ms Any progress in those discussions?

[zhiyuanliang-ms]

@McDoit
We are considering exposing the Feature Manager to public and simplify the required parameters of its constructor.
Something like this:

public class FeatureManager : IFeatureManager, IDisposable, IVariantFeatureManager
{
    private readonly TimeSpan ParametersCacheSlidingExpiration = TimeSpan.FromMinutes(5);
    private readonly TimeSpan ParametersCacheAbsoluteExpirationRelativeToNow = TimeSpan.FromDays(1);

    private readonly IFeatureDefinitionProvider _featureDefinitionProvider;
    private readonly FeatureManagementOptions _options;
    private readonly ConcurrentDictionary<string, IFeatureFilterMetadata> _filterMetadataCache;
    private readonly ConcurrentDictionary<string, ContextualFeatureFilterEvaluator> _contextualFeatureFilterCache;

    private class ConfigurationCacheItem
    {
        public IConfiguration Parameters { get; set; }

        public object Settings { get; set; }
    }

    public FeatureManager(
        IFeatureDefinitionProvider featureDefinitionProvider,
        FeatureManagementOptions options)
    {
        _featureDefinitionProvider = featureDefinitionProvider;
        _options = options ?? throw new ArgumentNullException(nameof(options));
        _filterMetadataCache = new ConcurrentDictionary<string, IFeatureFilterMetadata>();
        _contextualFeatureFilterCache = new ConcurrentDictionary<string, ContextualFeatureFilterEvaluator>();
    }

    public IEnumerable<IFeatureFilterMetadata> FeatureFilters;

    public IMemoryCache ParametersCache;

    public IEnumerable<ISessionManager> SessionManagers;

    public ILogger featureManagerLogger;

    public IEnumerable<ITelemetryPublisher> TelemetryPublishers { get; init; }

    public IConfiguration Configuration { get; init; }

    public ITargetingContextAccessor TargetingContextAccessor { get; init; }

    public TargetingEvaluationOptions AssignerOptions;
    
    // Methods
}

The AddFeatureManagement method provided by the lib will be like:

public static IFeatureManagementBuilder AddFeatureManagement(this IServiceCollection services)
{
    services.AddLogging();

    services.AddMemoryCache();

    //
    // Add required services
    services.TryAddSingleton<IFeatureDefinitionProvider, ConfigurationFeatureDefinitionProvider>();

    services.AddSingleton(sp => new FeatureManager(
        sp.GetRequiredService<IFeatureDefinitionProvider>(),
        sp.GetRequiredService<IOptions<FeatureManagementOptions>>()?.Value)
    {
        FeatureFilters = sp.GetService<IEnumerable<IFeatureFilterMetadata>>(),
        ParametersCache = sp.GetService<IMemoryCache>(),
        SessionManagers = sp.GetService<IEnumerable<ISessionManager>>(),
        featureManagerLogger = sp.GetService<ILoggerFactory>().CreateLogger<FeatureManager>(),
        Configuration = sp.GetService<IConfiguration>(),
        TargetingContextAccessor = sp.GetService<ITargetingContextAccessor>(),
        TelemetryPublishers = sp.GetService<IOptions<FeatureManagementOptions>>()?.Value.TelemetryPublisherFactories?
            .Select(factory => factory(sp))
            .ToList(),
        AssignerOptions = sp.GetService<IOptions<TargetingEvaluationOptions>>()?.Value
    });

    services.TryAddSingleton<IFeatureManager>(sp => sp.GetRequiredService<FeatureManager>());

    services.TryAddSingleton<IVariantFeatureManager>(sp => sp.GetRequiredService<FeatureManager>());

    services.AddSingleton<ISessionManager, EmptySessionManager>();

    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>();

    builder.AddFeatureFilter<ContextualTargetingFilter>();

    return builder;
}

Besides, we also want to provide a method called AddScopedFeatureManagement() which will register these services as scoped for you.

Currently, I am investigating the other DI containers in .NET including Autofac, Unity, Ninject and Castle Windsor. So far, I found that the proposed change of Feature Manager can be supported by all of DI libs mentioned above. More discussion is still needed to finalize the change. Supporting Blazor and other DI containers has been placed in our plan, but I am not very sure when it will be published. Sorry for the inconvinence.

[zhiyuanliang-ms]

Update:
The PRs of exposing FeatureManager to public and adding AddScopedFeatureManagement() method have been merged into main branch.
There is another PR #306 to demonstrate how to use FeatureManagement in Blazor server app.
The plan is to release these updates in 3.1.0

[samgibsonmoj]

Update: The PRs of exposing FeatureManager to public and adding AddScopedFeatureManagement() method have been merged into main branch. There is another PR #306 to demonstrate how to use FeatureManagement in Blazor server app. The plan is to release these updates in 3.1.0

Thank you.

[zhiyuanliang-ms]

Fixed in release 3.1.0