atifaziz / NCrontab

Crontab for .NET
Apache License 2.0
913 stars 139 forks source link

Implementation diverges from classic crontab syntax for "first *-day of month" #107

Open Gerrit-K opened 1 year ago

Gerrit-K commented 1 year ago

In https://github.com/atifaziz/NCrontab/issues/63#issuecomment-637545877 it was stated that

This library only supports the original contrab expression. [...] A first Tueday of a month can only occur on days 1-7, so you could use the 0 8 1-7 * Tue expression to effectively get 8 AM on first seven days of any month that's a Tuesday.

However, according to the crontab manpage:

Note: The day of a command's execution can be specified in the following two fields — 'day of month', and 'day of week'. If both fields are restricted (i.e., do not contain the "" character), the command will be run when either field matches the current time. For example, "30 4 1,15 5" would cause a command to be run at 4:30 am on the 1st and 15th of each month, plus every Friday.

Applied to the first example above, 0 8 1-7 * Tue, that would be "At 08:00 on every day-of-month from 1 through 7 and on Tuesday.", which is also confirmed by crontab.guru.

There's a detailed blog post about this and some funky way to work around it, but these workarounds produce exceptions in this library.

I don't think this is something that is easy to fix without breaking clients, and something that perhaps shouldn't be fixed, since it's more of a quirk in the original syntax. However, for me this was quite unexpected and I wasn't able to find much information about it, so I thought it might be worth pointing out, in case it should be included in the documentation.

atifaziz commented 1 year ago

perhaps shouldn't be fixed, since it's more of a quirk in the original syntax. However, for me this was quite unexpected and I wasn't able to find much information about it, so I thought it might be worth pointing out, in case it should be included in the documentation.

Thanks for bringing this to my attention!

A potential workaround for or-ing together schedules could be to merge the occurrences from each schedule like so:

// Requires following packages:
//
// - morelinq, 3.3.2: https://www.nuget.org/packages/morelinq/3.3.2
// - System.Interactive, 6.0.1: https://www.nuget.org/packages/System.Interactive/6.0.1

var start = new DateTime(2023, 1, 1).AddTicks(-1);

var s1 = CrontabSchedule.Parse("0 8 1-7 * *"); // 8 AM on first 7 days of any month
var s2 = CrontabSchedule.Parse("0 8 * * Tue"); // 8 AM on Tuesdays

var times1 = s1.GetNextOccurrences(start, DateTime.MaxValue);
var times2 = s2.GetNextOccurrences(start, DateTime.MaxValue);

var dates =
    times1.SortedMerge(MoreLinq.OrderByDirection.Ascending, times2) // morelinq
          .DistinctUntilChanged() // System.Interactive
          .Select(dt => dt.ToString("ddd, dd MMM yyyy HH:mm:ss"));

foreach (var dt in dates.Take(20))
    Console.WriteLine(dt);

outputs:

Sun, 01 Jan 2023 08:00:00
Mon, 02 Jan 2023 08:00:00
Tue, 03 Jan 2023 08:00:00
Wed, 04 Jan 2023 08:00:00
Thu, 05 Jan 2023 08:00:00
Fri, 06 Jan 2023 08:00:00
Sat, 07 Jan 2023 08:00:00
Tue, 10 Jan 2023 08:00:00
Tue, 17 Jan 2023 08:00:00
Tue, 24 Jan 2023 08:00:00
Tue, 31 Jan 2023 08:00:00
Wed, 01 Feb 2023 08:00:00
Thu, 02 Feb 2023 08:00:00
Fri, 03 Feb 2023 08:00:00
Sat, 04 Feb 2023 08:00:00
Sun, 05 Feb 2023 08:00:00
Mon, 06 Feb 2023 08:00:00
Tue, 07 Feb 2023 08:00:00
Tue, 14 Feb 2023 08:00:00
Tue, 21 Feb 2023 08:00:00

This will be easier (without the need for external dependencies) once PR #37 is merged.

Gerrit-K commented 1 year ago

Hey @atifaziz, thanks for the reply and the described workaround :) From an SDK/API perspective, this makes sense, but unfortunately I'm not directly using your library in code, so it's not possible for me to use it. I'm using schedules in Azure Pipelines which apparently use this library under the hood.

However, I actually wanted to use the AND version (e.g. every first Tuesday of the month), so I don't really needed a workaround. The solution 0 8 1-7 * Tue works perfectly fine for our schedule. I was just surprised that it doesn't adhere to the default cron implementation in that case and decided to report it in case someone else ends up in the same rabbit hole :wink: