Allow NCronJob access to IConfiguration

[falvarez1]

Objective:

Enhance the configuration process of NCronJob by proposing two new methods to integrate IConfiguration access into the AddNCronJob method. This change aims to reduce boilerplate code, streamline the configuration process for developers, and lay the foundation for easily integrating future features that require configuration. By centralizing configuration access, we can more effectively manage and introduce new settings and features without disrupting the existing implementation or increasing the complexity for developers.

Proposed Enhancements

Method 1: Optional Parameter (Non-breaking Change) Introduce an optional parameter to the existing AddNCronJob method that allows passing IConfiguration explicitly. This approach maintains compatibility with current usage while offering flexibility for developers who wish to manage their configuration more directly.

public static IServiceCollection AddNCronJob(this IServiceCollection services, Action<NCronJobOptionBuilder>? options = null, IConfiguration? configuration = null)
{
    var configuration = builder.Configuration;

    var concurrencySettings = new ConcurrencySettings();
    configuration.GetSection("NCronJob:Concurrency").Bind(concurrencySettings);

    ...

    return services;
}

// would be used like this:
builder.Services
        .AddNCronJob(n =>
            n.AddJob<PrintHelloWorldJob>(p =>
                p.WithCronExpression("*/2 * * * *"))
            , builder.Configuration);

Method 2: Extend WebApplicationBuilder (Breaking Change) Propose a more integrated approach by making AddNCronJob an extension of WebApplicationBuilder. This method inherently accesses IConfiguration directly within the extension, simplifying the API and aligning with ASP.NET Core configuration practices.

public static WebApplicationBuilder AddNCronJob(this WebApplicationBuilder builder, Action<NCronJobOptions> configure)
{
    var services = builder.Services;
    var configuration = builder.Configuration;

    ...

    return builder;
}

// would be used like this:
builder.AddNCronJob(n =>
        n.AddJob<PrintHelloWorldJob>(p =>
            p.WithCronExpression("*/2 * * * *"))
    );

[linkdotnet]

My current way of thinking was that the user should be responsible for all configuration and just pass down the values to us that are necessary. While I do see an appeal for the second approach (which I would prefer) I am still not totally convinced that we should push this right now.

@falvarez1, can you elaborate on where this either makes our current code much simpler or allows (right now) new scenarios? Don't get me wrong - we can think of this for v3 or vNext, but I am not 100% convinced.

[falvarez1]

This is mostly for paving the way to future enhancements. For example the dashboard component needs a way to provide a URI and some type of secret or access token. This would differ based on environment and infrastructure, you would want devs to have access to make changes in real-time without recompiling. Sure the developers could create their own way to push configuration into the extension methods but I think the dev experience starts to suffer. Another example would be to disable a particular job or updating the schedule just by updating a configuration. If this is used to create a Job Host that runs many jobs, it's not convenient to restart the service, potentially cancelling running jobs, just to make an adjustment to a Cron expression or to disable/enable a Job.

Regarding simplifying code, it wouldn't necessarily simplify the internal library code but the second option would just reduce boilerplate anywhere we would need access to IConfiguration.

[linkdotnet]

Really good thoughts and I do agree - for the time being I will flag this as „Future“.

[linkdotnet]

I moved this to v3 as we might have to introduce a breaking change here. Of course we could pass in a IConfiguration? config but that seems rather dirty.

[linkdotnet]

Potential first candate: MaxDegreeOfParallelism

[falvarez1]

@linkdotnet I've update your branch for global-configurable https://github.com/NCronJob-Dev/NCronJob/commit/4fc5ae0edcf478114e5ef6e886e031fb8110bf78

This should handle the scenario where calling the Services.NCronJob() multiple times affects the settings that are used in the Job's concurrency. This works with both providing the Action<GlobalOptions> either in the first call or any other call. This also works with IConfiguration.

If AddNCronJob is called on the WebApplicationBuilder then it will check if there's a record in the appsettings. The order is:

1. The configuration values from appsettings.json are bound to GlobalOptions.
2. Any additional configuration provided through the configureGlobalOptions parameter can override these values.

If 'AddNCronJob' is called from the IServiceCollection then everything works as it does today. If configureGlobalOptions is passed then this will override any defaults.

[nulltoken]

How about replicating the way IServiceCollection.AddSingleton() works? There's an extension that accepts a IServiceProvider. This way, the user could either resolve a IConfiguration or a bound xxxOptions directly?

Another option would be to split through the AddNCronJob()/UserNCronJob() pattern. The first taking care of feeding the DI container and the second would actually perform the job registration (and then, could easily access the ServiceProvider).

[linkdotnet]

I do like the second approach, just because it is more what people are used to!

[nulltoken]

If that's ok with you 'm going to try and give it a go. I'll ping back with an early spike to gather feedback.

[linkdotnet]

If that's ok with you 'm going to try and give it a go. I'll ping back with an early spike to gather feedback.

Ah absolutely. That would be awesome.

If possible, the solution should be backward compatible (semantic versioning). Otherwise, we have to target this feature for v4.

[nulltoken]

If possible, the solution should be backward compatible (semantic versioning). Otherwise, we have to target this feature for v4.

I've thought about that. Hence the "gather feedback" step :wink:

[nulltoken]

@linkdotnet I followed a quick (and somewhat dirty) track that led me into an dead-end, locked in the DI lifecycle.

If can think at least of two options:

• No breaking change:

  • From the user standpoint, we add an overload to NCronJob where options accepts both a NCronJobBuilder and a IServiceProvider
  • We untangle the NCronJobBuilder so that it doesn't register the types but only deal with the execution configuration.
  • options wouldn't be an Action<> anymore, but an Expression<Action<NCronJobBuilder, IServiceProvider>>
  • We build a visitor that would parse the Expression, and dynamically register the exposed types
  • The visitor would be run early, during the call to AddNCronJob
  • The configuration execution would be run through an internal IConfigureOptions<> that could access the IServiceProvider once it's built

• Breaking change:

  • AddNcronJob wouldn't accept any parameter anymore
  • Through some dumb scanner, it would find out all the types in the loaded Assemblies in the CurrentDomain and extract all user types implementing NCronJob interfaces (IJob, INotificationHandler, ...) and blindly register them, along with the standard NCronJob types (IRegistry, ...)
  • We would also register an empty JobDefinitionsContainer
  • We introduce a new UseNCronJob that would expose the previous configuration oriented interfaces where we could easily add the IServiceProvider as an optional parameter to the Action<>
  • During its execution, we resolve the JobDefinitionContainer and feed it with all the produced JobDefinitions

Obviously, the No breaking change path may need some more involved work (especially with the Expression visitor). The breaking change option comes with the downside of potentially registering user types that won't ever be used.

Before diving deeper, I was willing to share my train of thoughts and get your feedback on the proposed approach (considering the timeline you might have planned for v4).

[linkdotnet]

In both cases: How would one define dependencies between jobs? Currently, a user can just do the following:

Services.AddNCronJob(s => s.AddJob<MyJob>(c => ...).ExecuteWhen(success: s => s.RunJob<AnotherJob>()));

In the breaking change version, would it be moved to UseNCronJob?

I am not a big fan of implicit scanning, mainly because it will probably make #54 harder in the long run and may or may not surprise people. Given that we only have one global option, I don't see the benefit right now (given the complexity of the implementation).

There is no timeline for v4, and if it fits, I am fine with having a breaking change again with the next release. The migration process is small.

[nulltoken]

In both cases: How would one define dependencies between jobs?

non breaking change option: in the same way it's currently done.

breaking change version: Services.UseNCronJob(s => s.AddJob<MyJob>(c => ...).ExecuteWhen(success: s => s.RunJob<AnotherJob>()));

I am not a big fan of implicit scanning, mainly because it will probably make https://github.com/NCronJob-Dev/NCronJob/issues/54 harder in the long run and may or may not surprise people.

I agree. Not a fan of scanning either. However, in the breaking change version, without implicit scanning, that would require the user to let AddNCronJob() know about those types, so they can be registered in the DI container. And those same types would be also have to be repeated to describe the chaining in UseNCronJob. Which would make a not very appealing API from a user standpoint.

In the end, I guess the only viable option is the non breaking change one. As pointed out earlier, it might require slightly more work to make it happen.

[linkdotnet]

In the end, I guess the only viable option is the non breaking change one. As pointed out earlier, it might require slightly more work to make it happen.

Fair. I am inclined to put this into the backlog and tackle it when it is really needed. There are other issues that are more important than this one. But thanks for shipping in @nulltoken

[nulltoken]

I'm interested in playing further with this idea on my spare time. I'll try to find some time to work on the Expression angled approach.

[linkdotnet]

Oh for sure! That is very nice and welcome :D

[nulltoken]

Another use case where the .Add()/.Use() pattern may be useful has been identified in #106.

JobExecutor relies on ActivatorUtilities.CreateInstance which goes in the way of making #54 happen.

Below the initial comment

Maybe add a .UseNCronJob() extension method that would harvest the JobRegistry
and ensure every described type has been previously registered.
Otherwise, throw an InvalidOperationException ("The following types haven't
been registered in the dependency injection container: ...").

And to make sure the library consumer always invoke .UseNCronJob(), maybe make the QueueWorker
check some kind of marker flag that .UseNCronJob() would set.
Otherwise, throw an InvalidOperationException (".UseNCronJob() is expected to be called
once the application is built.").

[linkdotnet]

I will flag this one for v4 as well!

[nulltoken]

• options wouldn't be an Action<> anymore, but an Expression<Action<NCronJobBuilder, IServiceProvider>>

FWIW I've quickly tested this approach and forgot to report here my findings. I've eventually hit a quick brick wall. Expressions don't play well with optional arguments.

CS0854 An expression tree may not contain a call or invocation that uses optional arguments

I'll revisit this later and see if switching to overloads may help. But that may be a tedious path...