Teach JobOptionBuilder to only accept a parameter #241

nulltoken · 2025-03-09T09:18:49Z

Pull request description

Follow up to #215 and a minor step toward #217

PR meta checklist

Pull request is targeted at main branch for code
Pull request is linked to all related issues, if any.

Code PR specific checklist

My code follows the code style of this project and AspNetCore coding guidelines.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
I have updated the appropriate sub section in the CHANGELOG.md.
I have added, updated or removed tests to according to my changes.
- All tests passed.

codecov · 2025-03-09T09:20:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines	Coverage Δ
...NCronJob/Configuration/Builder/JobOptionBuilder.cs	`100.00% <100.00%> (ø)`
src/NCronJob/Registry/JobRegistry.cs	`91.81% <100.00%> (+1.19%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

linkdotnet · 2025-03-09T09:34:52Z

tests/NCronJob.Tests/JobOptionBuilderTests.cs

+    [Fact]
+    public void ShouldCreateMultipleParameterizedJobsWithAnd()
+    {
+        var builder = new JobOptionBuilder();


I understand the problems behind WithParameter().WithParameter - but when would a user anyway use WithParameter().And.WithParameter?
How would he trigger each of them without a cron expression nor a name?

You're right!

Indeed, the main use case for this is the one described in the doco

It's also possible to configure a job with only a parameter and no cron expression. Services.AddNCronJob(options => { options.AddJob<MaintenanceJob>(j => { j.WithParameter("lightMode"); }); }); This can later be triggered through the `IInstantJobRegistry` in a preconfigured mode (See [_"Instant jobs"_](./instant-jobs.md)). app.MapPost("/maintenance/light", (IInstantJobRegistry jobRegistry) => { // "lightMode" will be passed as a parameter to the job jobRegistry.RunInstantJob<MaintenanceJob>(); return Results.Ok(); }); Of course, the preconfigured parameter can also be overriden when triggering the job. app.MapPost("/maintenance/thorough", (IInstantJobRegistry jobRegistry) => { jobRegistry.RunInstantJob<MaintenanceJob>("thoroughMode"); return Results.Ok(); });

Indeed, the And. brings up some weird possible usages.
But I don't think that should be solved at the Fluent approach level, as It's also possible to trigger this behavior without the And.

builder .WithParameter("foo"); builder .WithParameter("bar");

Let me see if I can protect the user from this interesting weird case....

Basically we'd need these tests to pass

[Fact] public void CanRegisterAJobWithAPreconfiguredParameter() { Action act = () => ServiceCollection.AddNCronJob(n => n.AddJob<DummyJob>(p => p.WithParameter("one"))); act.ShouldNotThrow(); } [Fact] public void RegisteringAmbiguousJobConfigurationsLeadToAnExceptionWhenRegistration() { Action act = () => ServiceCollection.AddNCronJob( n => n.AddJob<DummyJob>(p => p .WithParameter("one") .And .WithParameter("two"))); act.ShouldThrow<InvalidOperationException>(); }

Great news! The first one already passes. That's 50% of the problem solved! 😉

Gave this a little bit of thought and added some more tests to better describe the behavioral contract.

Found one where the And. doesn't seem too "stupid"

[Fact] public void CanRegisterAJobWithMultipleDifferentlyParameterizedJobsAsStartupJobs() { Action act = () => ServiceCollection.AddNCronJob( n => n.AddJob<DummyJob>(p => p .WithParameter("one") .And .WithParameter("two")).RunAtStartup()); act.ShouldNotThrow(); }

Lol :D I am confused what I should expect here. Do we now have two startup jobs?

Yep. That's how And and RunAtStartup combine together.
FWIW, I'm not a big fan of RunAtStartup(), I'd rather see it as part of the JobOptionBuilder type

eg.

n => n.AddJob<DummyJob>(p => p .WithParameter("one") .RunAtStartup() .And .WithParameter("two") .RunAtStartup());

Proposal: Breaking change for V5?

I am really not sure if this is too implicit.
So, that we configure a job and trigger an instant job. How could the user override it? What if the user wants an explicit >null? How can we distinguish this behavior?

We currently can't because of how IJobExecutionContext is designed.

public interface IJobExecutionContext { [...] /// <summary>The passed in parameters to a job.</summary> object? Parameter { get; } [...] }

Would this interface be turned into something like this

public interface IJobExecutionContext { [...] /// <summary>The passed in parameters to a job.</summary> Parameter? AnotherNameThanParameterToEaseTheUserMigrationFromV4ToV5 { get; } [...] } public record Parameter(object? Value) { }

It would be possible for jobs to detect if a parameter has been explicitly passed.

Proposal: Breaking change for V5?

This doesn't look very intuitive (looks clunky)

n => n.AddJob<DummyJob>(p => p .WithParameter("one") .RunAtStartup() .And .WithParameter("two") .RunAtStartup());

I mean if we're introducing breaking changes anyways then just make it clear, like:

n => n.AddStartupJob<DummyJob>(p => p .WithParameters("one", "two",...));

Or we can have a separate "triggers" mechanic (like quartz, hangfire, temporal, elsa etc.) and designate the specific job to trigger on application "startup". A separate "triggers" mechanic would also allow future functionality necessary to trigger jobs based on external events like Webhooks etc. This is a separate and much bigger conversation if you guys are interested I have ideas on how this can be achieved.

I mean if we're introducing breaking changes anyways then just make it clear, like:

That wouldn't be a breaking change from an API point of view :D

But before adding a new API, it might be worth taking a step back and having an idea of what people really need and have a "better" version for v5.

Potential move of RunAtStartup() into a JobOption is tracked through #250

nulltoken · 2025-03-10T20:31:50Z

tests/NCronJob.Tests/IntegrationTests.cs

+            s => s.AddJob<DummyJob>(p => p
+                    .WithParameter("one"))
+        },
+        // TODO: Re-implement this test in a UseNCronJob context


@linkdotnet There might be a way to work around this, but it would trigger a some rewrite of the fluent interface. Basically, not storing the jobdefinitions during the AddJob() call, but collect them, have RunAtStartup() execute against those and then add them to the jobregistry.

Not impossible, but not trivial either.

I'd rather postpone until V5. Besides, with #219 we'll have to revisit this whole fluent thingie

Thoughts?

Basically as discussed in #221
If we have the "new" semantics we can do everything inside AddNCronJob

Not impossible, but not trivial either.

Done with #255

linkdotnet · 2025-03-10T21:02:42Z

docs/features/parameters.md

@@ -43,7 +43,41 @@ public class ReportJob : IJob
 }
 ```

+It's also possible to configure a job with only a parameter and no cron expression.


I am really not sure if this is too implicit.
So, that we configure a job and trigger an instant job. How could the user override it? What if the user wants an explicit null? How can we distinguish this behavior?

Configuring a Job with a parameter and allowing its override at runtime is already supported.

See

NCronJob/tests/NCronJob.Tests/IntegrationTests.cs

Lines 109 to 125 in ed0f4ce

[Fact]

public async Task InstantJobCanOverrideInitiallyDefinedParameter()

{

ServiceCollection.AddNCronJob(

n => n.AddJob<DummyJob>(o => o.WithCronExpression(Cron.Never).WithParameter("Hello from AddNCronJob")));

await StartNCronJob(startMonitoringEvents: true);

var orchestrationId = ServiceProvider.GetRequiredService<IInstantJobRegistry>().RunInstantJob<DummyJob>("Hello from InstantJob", CancellationToken);

await WaitForOrchestrationCompletion(orchestrationId, stopMonitoringEvents: true);

var filteredOrchestrationEvents = Events.FilterByOrchestrationId(orchestrationId);

filteredOrchestrationEvents.ShouldBeInstantThenCompleted<DummyJob>();

Storage.Entries[0].ShouldBe("DummyJob - Parameter: Hello from InstantJob");

Storage.Entries.Count.ShouldBe(1);

However, handling null explicitly as a parameter value looks like a new requirement.

I have no idea how to weigh how useful that could be (I've haven't designed any job in such a way).
If you believe this should be handled, let's create an issue and try and make it happen.

Test

NCronJob/tests/NCronJob.Tests/RuntimeJobRegistryTests.cs

Line 312 in e93ccfe

public async Task UpdatingParameterDoesNotSupportNullifying()

has been added in #249

Tracked through #251

linkdotnet · 2025-03-10T21:03:27Z

docs/features/parameters.md

+app.MapPost("/maintenance/light", (IInstantJobRegistry jobRegistry) =>
+{
+    // "lightMode" will be passed as a parameter to the job
+    jobRegistry.RunInstantJob<MaintenanceJob>();


But we don't have a way of having "nothing" as parameter.

Test

NCronJob/tests/NCronJob.Tests/RuntimeJobRegistryTests.cs

Line 312 in e93ccfe

public async Task UpdatingParameterDoesNotSupportNullifying()

has been added in #249

Tracked through #251

nulltoken · 2025-04-06T19:03:54Z

tests/NCronJob.Tests/IntegrationTests.cs

+        },
+        {
+            // Pure duplicate registration
+            s => s.AddJob<DummyJob>(p => p


With #255 merged, this test now pass. As such, I've resurrected this PR.

nulltoken · 2025-04-14T17:20:40Z

@linkdotnet Thoughts?

linkdotnet

Small change for the boolean "tree" :D will approve, but that change would be nice.

@nulltoken much appreciate your work and I am really sorry that I currently can't spare much time and you have to wait very long for a PR review.
I am a bit "overwhelmed" with stuff (more in a positive way). Currently, there is a lot of stuff on my table and I don't have too much time and energy left to take that hour and go through stuff. I know that is by far not the answer you are looking for, but I just wanted to be absolutely transparent.

Unfortunately, sometimes Open Source has to take a back seat in priorities and I hope that I can find some more time in the near future. There are some stuffs upcoming like conferences and so on - afterwards it should look a bit better.

Long story short: Thanks @nulltoken for offering all your valuable time to the project! Really appreaciate that and if there is something on how I can compensate you, let me know.

linkdotnet · 2025-04-14T18:02:30Z

src/NCronJob/Registry/JobRegistry.cs

@@ -155,6 +156,29 @@ Job registration conflict detected. A job has already been registered with the n
            """);
    }

+    private void AssertOnlyOneUnnamedUnscheduledParameterizedTypedJob(JobDefinition jobDefinition)
+    {
+        if (jobDefinition.CustomName is not null


Can we put this onto the JobDefintion type? That would be then also super trivial to unit test

I will. But this will be one hell of a property: IsUnnamedOrUnscheduledOrParameterlessTypedJob 😉

linkdotnet reviewed Mar 9, 2025

View reviewed changes

nulltoken force-pushed the em/parameters branch 3 times, most recently from 48f3448 to a8d502e Compare March 9, 2025 11:52

nulltoken changed the title ~~Teach JobOptionBuilder to only accept a parameter~~ [WIP] Teach JobOptionBuilder to only accept a parameter Mar 9, 2025

nulltoken force-pushed the em/parameters branch 2 times, most recently from f136a87 to 78c520c Compare March 9, 2025 19:26

nulltoken mentioned this pull request Mar 9, 2025

Teach JobOptionBuilder to add a cron expression to a named job #244

Merged

8 tasks

nulltoken force-pushed the em/parameters branch 3 times, most recently from 115d25b to 4402ea0 Compare March 10, 2025 20:21

nulltoken commented Mar 10, 2025

View reviewed changes

nulltoken force-pushed the em/parameters branch from 4402ea0 to 3074ce4 Compare March 10, 2025 20:33

linkdotnet reviewed Mar 10, 2025

View reviewed changes

This was referenced Mar 23, 2025

[V5] Replace RunAtStartup() method with a .RunAtStartup() JobOption #250

Open

[V5] Make it possible to deal with null parameters #251

Open

nulltoken added 2 commits April 6, 2025 20:36

Reorganize tests

5dd4258

Teach JobOptionBuilder to only accept a parameter

1d81184

nulltoken force-pushed the em/parameters branch from 3074ce4 to 1d81184 Compare April 6, 2025 18:44

nulltoken commented Apr 6, 2025

View reviewed changes

nulltoken changed the title ~~[WIP] Teach JobOptionBuilder to only accept a parameter~~ Teach JobOptionBuilder to only accept a parameter Apr 6, 2025

nulltoken requested a review from linkdotnet April 6, 2025 19:10

linkdotnet approved these changes Apr 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teach JobOptionBuilder to only accept a parameter #241

Teach JobOptionBuilder to only accept a parameter #241

nulltoken commented Mar 9, 2025 •

edited

Loading

codecov bot commented Mar 9, 2025 •

edited

Loading

linkdotnet Mar 9, 2025

nulltoken Mar 9, 2025

nulltoken Mar 9, 2025

nulltoken Mar 9, 2025

nulltoken Mar 9, 2025

nulltoken Mar 12, 2025

falvarez1 Mar 13, 2025 •

edited

Loading

falvarez1 Mar 13, 2025

linkdotnet Mar 13, 2025

nulltoken Apr 6, 2025

nulltoken Mar 10, 2025 •

edited

Loading

linkdotnet Mar 11, 2025

nulltoken Apr 6, 2025

linkdotnet Mar 10, 2025

nulltoken Mar 10, 2025 •

edited

Loading

nulltoken Apr 6, 2025

linkdotnet Mar 10, 2025

nulltoken Apr 6, 2025

nulltoken Apr 6, 2025

nulltoken commented Apr 14, 2025

linkdotnet left a comment

linkdotnet Apr 14, 2025

nulltoken Apr 14, 2025

	[Fact]
	public async Task InstantJobCanOverrideInitiallyDefinedParameter()
	{
	ServiceCollection.AddNCronJob(
	n => n.AddJob<DummyJob>(o => o.WithCronExpression(Cron.Never).WithParameter("Hello from AddNCronJob")));

	await StartNCronJob(startMonitoringEvents: true);

	var orchestrationId = ServiceProvider.GetRequiredService<IInstantJobRegistry>().RunInstantJob<DummyJob>("Hello from InstantJob", CancellationToken);

	await WaitForOrchestrationCompletion(orchestrationId, stopMonitoringEvents: true);

	var filteredOrchestrationEvents = Events.FilterByOrchestrationId(orchestrationId);
	filteredOrchestrationEvents.ShouldBeInstantThenCompleted<DummyJob>();

	Storage.Entries[0].ShouldBe("DummyJob - Parameter: Hello from InstantJob");
	Storage.Entries.Count.ShouldBe(1);

Teach JobOptionBuilder to only accept a parameter #241

Are you sure you want to change the base?

Teach JobOptionBuilder to only accept a parameter #241

Conversation

nulltoken commented Mar 9, 2025 • edited Loading

Pull request description

PR meta checklist

Code PR specific checklist

codecov bot commented Mar 9, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falvarez1 Mar 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken commented Apr 14, 2025

linkdotnet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken commented Mar 9, 2025 •

edited

Loading

codecov bot commented Mar 9, 2025 •

edited

Loading

falvarez1 Mar 13, 2025 •

edited

Loading

nulltoken Mar 10, 2025 •

edited

Loading

nulltoken Mar 10, 2025 •

edited

Loading