Job Filter as a Service

[pieceofsummer]

The current implementation of job filters suggests two possible ways of using them:

• Local filters added as attributes to Job class/method, - or -
• Global filters' instances added to GlobalJobFilters.Filters collection.

Both these ways appear to be virtually useless on .NET Core, because none of them provides any relevant information about current execution context. Since there's no HttpContext.Current and the likes available anymore (because of non-async nature, and the ugly looks too, I suppose :) ), so accessing anything beyond job filter's filterContext argument becomes quite troublesome.

The following PR provides a way to store job filters in a DI container, and to interact with other services.

For example, you might want to capture username (role, IP address, cookie, etc.) of the user who initiated the background job:

/// <summary>
/// Sample job filter implemented as a service
/// </summary>
public class CaptureUserNameJobFilter : IJobFilter, IClientFilter
{
    private readonly IHttpContextAccessor _contextAccessor;

    public CaptureUserNameJobFilter(IHttpContextAccessor contextAccessor)
    {
        if (contextAccessor == null)
            throw new ArgumentNullException(nameof(contextAccessor));

        _contextAccessor = contextAccessor;
    }

    public bool AllowMultiple => false;

    public int Order => JobFilter.DefaultOrder;

    public void OnCreating(CreatingContext filterContext)
    {
        var httpContext = _contextAccessor.HttpContext;
        if (httpContext == null)
            throw new InvalidOperationException("HttpContext is not available");

        filterContext.SetJobParameter("CurrentUser", httpContext.User.Identity.Name);
    }

    public void OnCreated(CreatedContext filterContext)
    {
    }
}

Now register it in Startup.cs:

public void ConfigureServices(IServiceCollection services)
{
    // ... ... ...
    // ... other services' configuration
    // ... ... ...

    services.TryAddEnumerable(ServiceDescriptor.Singleton<IJobFilter, CaptureUserNameJobFilter>());
}

And... voila! All registered services implementing IJobFilter will be treated as job filters.

---

P.S. The only thing is, you cannot use BackgroundJob / RecurringJob static methods anymore, because they use a private (non-DI) instance of client class, hence they will ignore the new lookup method.

You'll need to inject a proper instance of IBackgroundJobClient / IRecurringJobManager and use its methods instead. It is not actually a bad thing, since it employs a typical .NET Core pattern instead of a dubious static wrapper, but the static wrapper still shouldn't produce "partial success" results (failing with exception would be better).

---

P.P.S. BackgroundJob and RecurringJob appear to use different ways of storing a local instance. Feels like BackgroundJob was designed with possibility to switch implementations in mind (even though it's not actually used), while RecurringJob never bothered to.

[odinserj]

There is the hidden Hangfire.Common.JobFilterProviders.Providers property you can use to add a custom filter provider that is based on DI. However I agree, that dependency injection should be tightly integrated with job filters, like in ASP.NET MVC. Needs some more investigation.

[pieceofsummer]

Yep, I know about JobFilterProviders.Providers, but it is essentially the same as GlobalJobFilters.Filters. You need to have a DI context to activate an instance of provider before you can add it there. But the thing is, you don't have a DI container until you finished setup, so you'll need to add it at some point after setup, which is not intuitive at all :)

Also a DI container (for scoped services) might be disposed and another one created (so the provider instance in global providers will be using a dead DI container, and the number of instances will keep growing). You'll need some non trivial scope tracking logic to handle this, or to limit the provider to singleton services only (don't know which one is worse).

[KorsG]

This would be extremely useful. +1

[maulik-modi]

@pieceofsummer and @odinserj ,
What is preventing from getting it merged?

[miroslavgrozdanovski]

Please consider this, it would be very useful.

[miroslavgrozdanovski]

@pieceofsummer, @odinserj,

I know this PR was created a long time ago, but wanting to implement it in our code, I just want to confirm one thing.

As the IJobFilterProvider is configured as singleton, wouldn't that mean that scoped dependencies cannot be correctly used in the client filters that are now resolved through DI?

Because the AspNetCoreJobFilterProviders class is a singleton, it will receive one Service Provider and it will always use that one. But knowing that each API request in AspNet Core is handled within a different scope, the scoped services injected in the Controller and in the Client Filter will end up being different, so you can't really pass on the logged in user that way.

One way of making this work is to make all dependencies to be scoped, which include the IBackgroundJobClient and IJobFilterProvider. That way the scope of whoever is injecting an IBackgroundJobClient will be used for creating the AspNetCoreJobFilterProviders class which then creates the filters (which will now use the same scoped services like for example IUserProvider).

Am I missing something?
Thanks!

[pieceofsummer]

I believe the example class set as a singleton was only because IHttpContextAccessor was a singleton, hence there was no reason to make it scoped. But there's really nothing preventing you from doing so if you need.

The PR would probably need some refactoring to match it with the recent code base changes before you can use it though.

[miroslavgrozdanovski]

I believe the example class set as a singleton was only because IHttpContextAccessor was a singleton, hence there was no reason to make it scoped. But there's really nothing preventing you from doing so if you need.

The PR would probably need some refactoring to match it with the recent code base changes before you can use it though.

@pieceofsummer, thanks for the quick answer!

I agree for the needed refactor to match the recent code, though the discussion around scoped dependencies will apply again.

Yes, you can easily register your filter as scoped if it needs scoped dependencies. What I want to say is that won't be enough, as you would need to register the AspNetCoreJobFilterProviders and IBackgroundJobClient as scoped as well in order to make it work.

That's why I would register those classes as scoped as part of this PR as well. Or think about some other solution. Otherwise, if people want scoped dependencies, they would need to read the Hangfire source code, figure this thing out and register these classes as scoped manually before calling AddHangfire. Thankfully this is doable as AddHangfire uses TryAddSingleton afterwards, but all of this is implementation details that should stay inside.

Does this make sense to you? is what I'm saying correct?
I'm just diving into the source code these days, trying to exactly figure out a solution for this.

[odinserj]

Starting from Hangfire 1.7.0 it's possible to use the IServiceProvider instance when calling the AddHangfire method during the configuration:

services.AddHangfire((provider, configuration) => configuration
    .UseServiceProviderJobFilterProvider(provider)
    .UseXXX()
    .UseZZZ()
    .UseYYYStorage());

So the UseServiceProviderJobFilterProvider method above can be implemented with the bits from this pull request in the following way. In this case every component in Hangfire will know about the new IJobFilterProvider so execution will be consistent across all the components, both client and server.

public class ServiceProviderJobFilterProvider : IJobFilterProvider
{
    private readonly IServiceProvider _services;

    public ServiceProviderJobFilterProvider([NotNull] IServiceProvider services)
    {
        _services = services ?? throw new ArgumentNullException(nameof(services));
    }

    public IEnumerable<JobFilter> GetFilters(Job job)
    {
        return _services.GetServices<IJobFilter>()
            .Select(x => new JobFilter(x, JobFilterScope.Global, null));
    }
}

public static class ServiceProviderJobFilterProviderExtensions
{
    public static IGlobalConfiguration UseServiceProviderJobFilterProvider(
        [NotNull] this IGlobalConfiguration configuration,
        [NotNull] IServiceProvider services)
    {
        JobFilterProviders.Providers.Add(new ServiceProviderJobFilterProvider(services));
        return configuration;
    }
}

[miroslavgrozdanovski]

@odinserj, it's a nice wrap up for filters using transient and singleton dependencies, but it looks like it won't work correctly with scoped services.

The way I have it working now is to manually register the AspNetCoreJobFilterProviders and IBackgroundJobClient as scoped, before calling AddHangfire. So something like this:

services.AddScoped<IJobFilterProvider>(x => new AspNetCoreJobFilterProviders(x));

services.AddScoped<IBackgroundJobClient>(x => new BackgroundJobClient(
    x.GetService<JobStorage>(),
    x.GetService<IJobFilterProvider>()
));

services.AddHangfire((provider, configuration) => configuration.UseXXXStorage());

I'm fine with this working for Client filters so far, as we have an additional issue with using DI in Server filters (a known one). As the OnPerforming methods are called before the scope is created for the job execution, we use a different workaround there altogether.

Please have these two things in mind, as merging this and saying that Hangfire now supports filters through DI would probably not be ideal without proper support for scoped dependencies.

[pieceofsummer]

@miroslavgrozdanovski I guess this is the reason why the latest version uses factories to construct IBackgroundJobClient and IRecurringJobManager instead of directly injecting them from container )

You can technically create a custom IBackgroundJobPerformer and construct DI scope early so that filters are executed within it. But that would require manual constructing of the BackgroundJobServer instead of using provided extension methods.

In my project I'm doing exactly this but for a different reason, to allow hot stop/restart server with new options when the relevant configuration file changes. Totally doable, but requires some extra work :)

[miroslavgrozdanovski]

@pieceofsummer, yeah, there a lot of options if you start overriding the classes, though that path will always be undocumented and fragile from future changes, as you are messing with the internals. It may be worth it for more custom requirements like yours, but it would be good if having proper DI (supporting scoped as well) in job filters is the default Hangfire behavior.

[odinserj]

@miroslavgrozdanovski please give an example of a custom filter that will not work with the UseServiceProviderJobFilterProvider extension method, but will work with your AddScoped suggestion.

[miroslavgrozdanovski]

@miroslavgrozdanovski please give an example of a custom filter that will not work with the UseServiceProviderJobFilterProvider extension method, but will work with your AddScoped suggestion.

Sure @odinserj , below is an example that just tries to pass on a value stored in a scoped class (let's say the ID of the logged in user).

public class UserFilter : IJobFilter, IClientFilter
{
    public bool AllowMultiple => false;
    public int Order => 1;

    private readonly IScopedService _scopedService;

    public UserFilter(IScopedService scopedService)
    {
        _scopedService = scopedService;
    }

    public void OnCreating(CreatingContext filterContext)
    {
        int userId = _scopedService.UserId;
        // Store it as a Job Parameter
    }

    public void OnCreated(CreatedContext filterContext)
    {
    }
}

This is the rest of the code that sets the UserId and tries to enqueue a job.

[ApiController]
[Route("[controller]")]
public class TestController : ControllerBase
{
    private readonly IBackgroundJobClient _backgroundJobClient;
    private readonly IScopedService _scopedService;

    public TestController(
        IBackgroundJobClient backgroundJobClient, 
        IScopedService scopedService)
    {
        _backgroundJobClient = backgroundJobClient;
        _scopedService = scopedService;
    }

    [HttpGet]
    public void TestHangfire()
    {
        _scopedService.UserId = 123;
        _backgroundJobClient.Enqueue<TestJob>(x => x.TestMethod());
    }
}

Using your proposed solution, the filter provider will use the root container. If my UserFilter is registered as Scoped, I'm getting this exception "Cannot resolve scoped service IEnumerable<IJobFilter> from root provider". If I register the UserFilter as transient, I'm still getting an exception because the filter depends on a scoped service: "Cannot resolve IEnumerable<IJobFilter> because it requires scoped service IScopedService".

I would have mentioned these exceptions earlier, but I only got them while trying to reproduce the issue on a new clean project that uses the basic DI container from .NET Core. Previously, I was using Lamar on my work project, so this exception wasn't occurring. But it still wasn't working, as the UserId was always 0 in the filter.

My solution of making the IJobFilterProvider and IBackgroundJobClient classes scoped ensures that the container injected in the filter provider will be the same as the one used throughout the API request (so I'll have the same instance of the IScopedService in the Controller and in the Filter).

[odinserj]

P.S. The only thing is, you cannot use BackgroundJob / RecurringJob static methods anymore, because they use a private (non-DI) instance of client class, hence they will ignore the new lookup method.

This is why I don't want to provide an out-of-box solution that's based on overridden IJobFilterProvider instance – sooner or later there will be confusion, because in this case BackgroundJob and IBackgroundJobClient will work differently. So if better solution exists, it should be chosen.

@miroslavgrozdanovski, thanks for the detailed description. Actually there's the IHttpContextAccessor interface that's used exactly for cases like this, and it contains the generic Items collection that can be used to pass data between different layers. For example consider the following controller:

[ApiController]
[Route("[controller]")]
public class TestController : ControllerBase
{
    private readonly IBackgroundJobClient _backgroundJobClient;

    public TestController(IBackgroundJobClient backgroundJobClient)
    {
        _backgroundJobClient = backgroundJobClient;
    }

    [HttpGet]
    public void TestHangfire()
    {
        HttpContext.Items.Add("UserId", 12345);
        _backgroundJobClient.Enqueue<TestJob>(x => x.TestMethod());
    }
}

To access the IHttpContext instance we should inject the IHttpContextAccessor instance as a constructor parameter and use the Items property:

public class UserFilter : IClientFilter
{
    private readonly IHttpContextAccessor _accessor;

    public UserFilter(IHttpContextAccessor accessor)
    {
        _accessor = accessor;
    }

    public void OnCreating(CreatingContext filterContext)
    {
        int userId = (int)_accessor.HttpContext.Items["UserId"];
        // Store it as a Job Parameter
    }

    public void OnCreated(CreatedContext filterContext)
    {
    }
}

The we should register the IHttpContextAccessor interface and our UserFilter instance:

services.AddHttpContextAccessor();
services.AddHangfire((provider, config) => config
    .UseXXXStorage()
    .UseFilter(new UserFilter(provider.GetRequiredService<IHttpContextAccessor>())));

And we can use a solution that will not be broken with future releases, because it's expected.