ScheduleJob shorthand: Job name should match trigger name by default

[Timovzl]

Describe the bug

ScheduleJob is documented as a shorthand for AddJob plus AddTrigger, and it is wonderful.

Configuring the trigger is mandatory. Configuring the job is optional. It is, by far, the most readable to omit the job configuration - even the example in the docs omits it.

However, if no job name is explicitly specified, a UUID is generated for it. This seems like a poor default. Consider the example from the docs:

q.ScheduleJob<ExampleJob>(trigger => trigger
    .WithIdentity("Combined Configuration Trigger")
    .StartAt(DateBuilder.EvenSecondDate(DateTimeOffset.UtcNow.AddSeconds(7)))
    .WithDailyTimeIntervalSchedule(x => x.WithInterval(10, IntervalUnit.Second))
    .WithDescription("my awesome trigger configured for a job with single call")
);

The trigger name will be "Combined Configuration Trigger". What should the job name be? I would infer that, considering what the shorthand was designed for in the first place, the job name should match the trigger name by default.

Version used

3.3.2.

To Reproduce

Simply implement the example from the docs.

Expected behavior

The job name (in the backing store) should equal the trigger name.

Additional Context

The only workaround we currently have is significantly uglier than the example, and seems to somewhat cancel out the benefits of the shorthand:

q.ScheduleJob<ExampleJob>(trigger => trigger
    .WithIdentity("Combined Configuration Trigger")
    .StartAt(DateBuilder.EvenSecondDate(DateTimeOffset.UtcNow.AddSeconds(7)))
    .WithDailyTimeIntervalSchedule(x => x.WithInterval(10, IntervalUnit.Second))
    .WithDescription("my awesome trigger configured for a job with single call"),
    job => job.WithIdentity("Combined Configuration Trigger")
);

More importantly, it violates the DRY (Don't Repeat Yourself) principle. We could store the job name in a variable or constant, but that would require even more code.

[lahma]

Thanks for the feedback, this is quite logical but also a breaking change, but again with GUID ids it doesn't make much of a breaking change until someone clashes with existing registration. Would you like to make a PR for this?

[Timovzl]

@lahma Sure, I will make a PR. I notice that I'll have to add a few properties, so that logic can be performed from the sensible place, but I can keep them internal and get-only.

[Timovzl]

@lahma I'm trying to add some unit tests to protect the new behavior going forward, but I'm running into the following issues:

• The unit test project does not reference Quartz.Extensions.DependencyInjection. Shall I add this project reference?
• The InternalsVisibleTo from Quartz to Quartz.Tests.Unit does not appear to be working.
• Somehow it seems impossible to implement or fake IServiceCollectionQuartzConfigurator, due to its internal IServiceCollection Services { get; }. We could make it protected internal, as far as I'm concerned, but not all of our target runtimes support it.

As a result, I'm unable to add any unit tests on ServiceCollectionExtensions.ScheduleJob<T>(). Could you assist?

[lahma]

Adding a new reference is fine, also more permitting visibility modifiers if so needed. I don't think framework versions should be s limiting factor here. The internals visible was working at some point at least IIRC.

[Timovzl]

Adding a new reference is fine, also more permitting visibility modifiers if so needed.

[assembly: InternalsVisibleTo("Quartz.Tests.Unit")]

Doing so, I run into this compiler error: Friend assembly reference 'Quartz.Tests.Unit' is invalid. Strong-name signed assemblies must specify a public key in their InternalsVisibleTo declarations.

@lahma What do you suggest? Do you know how I can obtain the correct public key, or do we have another way to handle this?

As a side note, I'm curious how a default interface implementation (which internal IServiceCollection Services { get; } is) can make it impossible to implement a public interface from another assembly. I'm guessing this may have something to do with the net461 target.

[lahma]

Now back at computer, easier to navigate the code.

So there's already definitions for the InternalsVisibleTo, see AssemblyInfoExtras.cs. But I think as it's not referenced by DI project so you can just add link to that existing file. The public/private key pair can be found from Quartz.snk from repository root, if ever needed.

I don' think internal IServiceCollection Services { get; } is a default implementation, it just defines that getter must be present?

[Timovzl]

I'm uncertain. I don't think you can specify an access modifier on regular (non-default) interface members. In other words, before default interface implementations, I believe such an access modifier would never compile.

I could be mistaken.

P.S. Meanwhile, I have circumvented the issue entirely, by simply making use of services.AddQuartz.

[Timovzl]

@lahma The PR is ready. It looks like I'm not permitted to push the feature branch, though. Or am I supposed to use a fork?

[lahma]

@Timovzl please use a fork, it's quite standard way as I cannot allow full write access to this repo for everyone. Using a PR allows us to discuss the implementation.

[lahma]

Thanks for the discussion and a great PR to improve things!