AnderssonPeter / Hangfire.Console.Extensions

Makes it easier to use Hangfire.Console with .net core dependency injection
MIT License
81 stars 17 forks source link

Feature: injection of `IPerformContext` #4

Closed MihailsKuzmins closed 2 years ago

MihailsKuzmins commented 3 years ago

IPerformingContextAccessor is a nice feature which helps with DI a lot, but still it makes mocking a bit inconvenient because the PerformingContext IPerformingContextAccessor.Get() must be set up which is not an interface, therefore, a new class PerformingContextMock must be created for this purpose.

Wouldn't it be more awesome if IPerformContext could be directly injected? In my project I created something like this: Interfaces

public interface IProgressReporter
{
    void SetValue(double value);
}

public interface IPerformContext
{
    void WriteLine(string format, params object[] args);

    IProgressReporter StartProgress(string name);
}

Implementations

internal sealed class ProgressReporter : IProgressReporter
{
    private readonly IProgressBar? _progressBar;

    public ProgressReporter(IProgressBar? progressBar)
    {
        _progressBar = progressBar;
    }

    public void SetValue(double value) =>
        _progressBar?.SetValue(value);
}

internal sealed class PerformContext : IPerformContext
{
    private readonly IPerformingContextAccessor _performingContextAccessor;

    public PerformContext(IPerformingContextAccessor performingContextAccessor)
    {
        _performingContextAccessor = performingContextAccessor;
    }

    public void WriteLine(string format, params object[] args) =>
        _performingContextAccessor.Get()?.WriteLine(format, args);

    public IProgressReporter StartProgress(string name)
    {
        var progressBar = _performingContextAccessor.Get()
            ?.WriteProgressBar(name);

        return new ProgressReporter(progressBar);
    }
}

I have to admit that it is still a bit unfinished because if IPerformingContextAccessor.Get() returns null (it means that a task is not scheduled but just called in debugging possibly) I wanted to make use of the Console.ProgressBar NuGet package.

If you think that this concept might be useful in Hangfire.Console.Extensions I guess I could help with a PR or something

mkgn commented 3 years ago

Hi,

Thought to write because I am also looking for a way to inject IPerformContext to my jobs. I downloaded the source but can't really figure out how it's being wired up. As of now I have your class HangfireContextFilter:IPerformingContextAccessor.

Then I have a client filter like;

public class HangfireJobGlobalContextInjector : IClientFilter
{
    private readonly IServiceProvider _serviceProvider;

        public HangfireJobGlobalContextInjector(IServiceProvider serviceProvider)
    {
        _serviceProvider = serviceProvider;
    }

    public void OnCreating(CreatingContext filterContext)
    {
        var httpContext = _serviceProvider.GetService<IHttpContextAccessor>().HttpContext;

        var tenant = httpContext.GetMultiTenantContext<AppTenant>().TenantInfo;
        var userId = httpContext.User.Identity.Name;

        //save common parameters needed for all the jobs in the application
        filterContext.SetJobParameter(HangFireConstants.TenantId, tenantData);
        filterContext.SetJobParameter(HangFireConstants.UserId, 1);
    }

    public void OnCreated(CreatedContext filterContext) { }
}

Then in my startup I wire these up like(actually taken from the code in this project);

        services.AddSingleton<IPerformingContextAccessor, HangfireContextFilter>();
        services.AddTransient(sp => sp.GetRequiredService<IPerformingContextAccessor>().Get());
        services.AddTransient<PerformingContext>(sp => sp.GetRequiredService<IPerformingContextAccessor>().Get());

and then register the filter like;

globalConfiguration.UseFilter(new HangfireJobGlobalContextInjector(provider));

Now, when a job is created it goes to the HangfireJobGlobalContextInjector client injector and saves the tenantId & user ID related to the job. Now my problem is getting your IPerformingContextAccessor injected to job.

In your code there is HangfireSubscriber as a singleton that seems to be the one controlling IPerformingContextAccessor. However I dont see any references to it (or registering it somewhere as a filter). So because of that I always get a null for job context and can't retrieve the saved data for the job.

Will you be able to help me wire this up properly so I can inject JobContext to my job classes.

MihailsKuzmins commented 3 years ago

@mkgn, I have these two interfaces (IProgressReporter and IPerformContext) and their implementation. Then I register IPerformContext in the DI (IProgressReporter does not need to be registered because it's created by StartProgress())

public class Startup
{
    public static void ConfigureServices(IServiceCollection services)
    {
        services.AddHangfire((provider, cfg) =>
        {
            var connectionString = provider.GetRequiredService<IConfiguration>()
                .GetConnectionString("Hangfire");

            cfg.UseSqlServerStorage(connectionString);
            cfg.UseSerilogLogProvider();
            cfg.UseConsole();
        });

        services
            .AddHangfireServer()
            .AddHangfireConsoleExtensions() // REGISTERS IPerformingContextAccessor (from Hangfire.Console.Extensions)
            .AddSingleton<IPerformContext, PerformContext>(); // THIS LINE REGISTERS IPerformContext
    }

    public static void Configure(IApplicationBuilder app, IWebHostEnvironment env)
    {
        if (env.IsDevelopment())
            app.UseDeveloperExceptionPage();

        app.UseRouting();
        app.UseHangfireDashboard();
    }
}

When the setup is complete I simply inject IPerformContext anywhere I want (well, when Hangfire jobs are run). You said that IPerformingContextAccessor had been null in your code, right? So I think that you haven't specified AddHangfireConsoleExtensions() (please see the code sample above). Apart from it I don't see any other problems.

If this does not clarify the issue, please let me know.

mkgn commented 3 years ago

Actually I am not using the extension. I just wanted to understand and use how you inject IPerformContext and use it in my code. Since you have the HangfireSubscriber as a service filter. I just added it to the global filters globalConfiguration.UseFilter(new HangfireSubscriber());

That worked and now my client filter saves the global job parameters before jobs are getting created. When job is executing I now can inject IPerformingContextAccessor to the job via constructor and retrieve the job parameters i saved.

Seems to work fine. My only worry is why you haven't registered it as a filter in your code. I am worried because even though it works for me, whether there is any hidden issues in the method i used(like giving wrong job parameters or mixing them up).

MihailsKuzmins commented 3 years ago

Since I use this library I use the extension method AddHangfireConsoleExtensions() as it is said in the README file to make the life more simple. If you use this NuGet as well, I advise you to do the same and not reinvent the wheel. Then you could use my classes for injecting IPerformContect directly.

But if you want to register all services on your own, you could take a look at the source code of AddHangfireConsoleExtensions() and do the same in your codebase. https://github.com/AnderssonPeter/Hangfire.Console.Extensions/blob/9d6dafb1d71deda4e5d518854e11da8451180827/Hangfire.Console.Extensions/HangfireExtensions.cs#L11-L23

As you see UseFilter(new HangfireSubscriber()) is added by AddHangfireConsoleExtensions(). By calling the extension method I do not need to worry about how these things work.

AnderssonPeter commented 3 years ago

Hi sorry for the late reply, but the PerformingContext does not have a interface. https://github.com/HangfireIO/Hangfire/blob/15665ce8dfa9c458cb855575d2ca90d7a1e191b1/src/Hangfire.Core/Server/PerformingContext.cs#L23 https://github.com/HangfireIO/Hangfire/blob/15665ce8dfa9c458cb855575d2ca90d7a1e191b1/src/Hangfire.Core/Server/PerformContext.cs#L30

If you can get hangfire it self to add a interface then i would love to add it in the dependency injection!

MihailsKuzmins commented 3 years ago

Hi @AnderssonPeter,

thank you for the reply. Yes, IPerformContext does not have an interface, so I needed to create a wrapper around it. So, I guess you do want this feature to be implemented, right? Will you do it yourself or are you expecting a PR?

AnderssonPeter commented 3 years ago

@MihailsKuzmins I think the right thing would be to send a PR to hangfire and once that is merged I can make the adjustments in this nuget.

MihailsKuzmins commented 3 years ago

@AnderssonPeter to be honest, I'm not really sure why you need an interface for PerformingContext. If we talk about your library IPerformingContextAccessor already does the whole job and this interface comes from your library not from the original Hangfire. I just think that a nice wrapper could be made around IPerformingContextAccessor to make calls and checks a bit more easier 🤔

AnderssonPeter commented 3 years ago

So instead of adding a interface, you want to add a interface and a class that just calls the original class?

That seems overly complicated?

Also this would abstract away any chance of casting to PerformingContext..

MihailsKuzmins commented 3 years ago

As for casting, what if _performingContextAccessor.Get() returns null? If it's not a problem we could write something like this

public static explicit operator PerformingContext?(PerformContext @this) =>
    _performingContextAccessor.Get();

As for being complicated, I don't think so, just 2 classes + 2 interfaces

AnderssonPeter commented 2 years ago

Sadly i still think its overkill to create 4 files instead of 1 with lots of complicated logic where none is needed.. So i will close this for now, if you or someone else manage to get the interface into the Hangfire codebase i will happily make the required changes here!