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.