kelektiv / node-cron

Cron for NodeJS.
MIT License
8.44k stars 621 forks source link

Incorrect Behavior in Setting Up a Cron Job for First Monday of the Month #890

Open mnkasikci opened 3 months ago

mnkasikci commented 3 months ago

Description

The cron expression ("5 12 /100,1-7 1") for running a cron job on every first Monday of the month at 12:05 PM appears to be having unintended behavior. The linkage between the month and weekday in the given expression is "OR" , but it should be "AND". (It works every monday + first 7 days of the month, but it should work only if it's monday AND if it's first seven days of the month) Sources: a blogpost Crontab.guru

Expected Behavior

The cron job should execute only on the first Monday of the month at 12:05 PM.

Actual Behavior

Instead of running exclusively on the first Monday of the month, the cron job is executing on every Monday and the first seven days of any month.

Possible Fix

No response

Steps to Reproduce

  1. Set the cron expression in your node-cron configuration to "5 12 */100,1-7 * 1".
  2. Create a cron job that runs the function which logs a message.
  3. Run the cron job.
  4. Check the output of the next 10 execution dates:
    const cronExpression = "5 12 */100,1-7 * 1";
    const job = new CronJob(
    cronExpression,
    function () {
    console.log("You probably won't wait this long, but if you do, you should see that it runs every monday + first seven days of the month.");
    console.log(`Run on : ${new Date().toISOString()}`);
      }, // onTick
    null, // onComplete
    true,
    );
    for (const nextDate of job.nextDates(10))
    console.log(nextDate.toString());

Context

I was trying to send a certain report to people every first monday of the month. As a temporary solution, i set the cron expression to run every monday , and the onTick() function checks if it's in the first 7 days of the month, and returns without doing nothing otherwise.

Your Environment

I don't think these are relevant, but still providing them

sheerlox commented 3 months ago

Hi, thanks for reporting!

I must admit my knowledge stopped here:

This expression translates to “Run at the midnight if it’s the first day of the month OR Monday”. An expression like this could be handy for a job that sends weekly and monthly reports as it probably needs to run at the start of every week, and the start of every month. However, if either field is unrestricted, the logical relationship between the fields changes to “AND”.

I wasn't aware that any of these fields starting with a * would still be considered unrestricted. This is super interesting.

However, this feels like a hack (or trick as the author names it). We have a feature request (as well as a draft PR) to implement the # character that would fit this purpose more properly, although not available in the standard cron syntax.

I'm all for keeping aligned with the UNIX standard. Still, I wonder if this particular use case, because it's so hacky, wouldn't be more fit to be disclosed as unsupported in the README, in favor of the # character.

What do you think?

mnkasikci commented 3 months ago

That would apparently fit better to the use case, easier to read and less prone to user error.

Telling cron to run every 100th day of the month is indeed really hacky. I hated every moment of writing it to my code, but that would be the lesser of two evils :) (the other being, checking if it's the proper time for running in the function )

All in all, I agree that the problems this feature would bring outweights the benefits, considering the draft pr you mentioned.

Thanks for your time