cron-validate icon indicating copy to clipboard operation
cron-validate copied to clipboard

Decimal values do not cause an error

Open annybs opened this issue 1 year ago • 7 comments

While testing some edge cases for our app, I have discovered that cron-validate allows values with decimals. Tested in Node v20 REPL:

> cron('0.5 1.5 2.5 3.5 4.5')
Valid {
  value: {
    seconds: undefined,
    minutes: '0.5',
    hours: '1.5',
    daysOfMonth: '2.5',
    months: '3.5',
    daysOfWeek: '4.5',
    years: undefined
  }
}

To my knowledge, this isn't valid cron syntax - at the very least I believe it would be non-standard?!

annybs avatar May 08 '24 09:05 annybs

I'll take a look, not sure right now if decimal numbers are allowed / non-standard tbh.

Airfooox avatar May 12 '24 15:05 Airfooox

Hi, I tested a */99 * * * * * which I think is also an invalid cron expression.

image

ArtyomCZ avatar May 20 '24 15:05 ArtyomCZ

So I'd guess the step value should be an integer, less than the maximum allowed value for that field and also be a even divisor (modulus = 0)?

Airfooox avatar May 20 '24 15:05 Airfooox

It seems hard to find a definite answer to what's supposed to be allowed for step values. I noticed that crontab.guru think's */99 * * * * should be a valid expression representing every 99th minute, but other than that I find nothing actually confirming if this should be valid. A quick search on Google on how to do a cron for every 90th minute gives all kinds of suggestions but never that */90 should work. Looking at other implementations like Hashicorps' cronexpr it seems like they simply reject steps larger than max allowed value.

Although there's no source cited, Wikipedia also includes a note about frequency:

Note that frequencies in general cannot be expressed; only step values which evenly divide their range express accurate frequencies (for minutes and seconds, that's /2, /3, /4, /5, /6, /10, /12, /15, /20 and /30 because 60 is evenly divisible by those numbers; for hours, that's /2, /3, /4, /6, /8 and /12); all other possible "steps" and all other fields yield inconsistent "short" periods at the end of the time-unit before it "resets" to the next minute, second, or day; for example, entering */5 for the day field sometimes executes after 1, 2, or 3 days, depending on the month and leap year; this is because cron is stateless (it does not remember the time of the last execution nor count the difference between it and now, required for accurate frequency counting—instead, cron is a mere pattern-matcher).

I think rejecting step values that's not an integer between 1 and maximum allowed value is the right thing to do and is what I'm looking for at least. Having the extra modulus check is a bonus I guess to not give surprises to the user, maybe as an opt-in.

bombsimon avatar Jun 03 '24 09:06 bombsimon

Just to pick up on the above comment this is currently causing a few issues with some validation I'm attempting to do where */200 is being validated as true when it's above the max limit. Is there any indication if this will be changed in the future?

invmatt avatar Jun 27 '24 19:06 invmatt

An update from my side: Im currently heavily invested in learning for my exams, but after they're done, I'll update the cron-validate with the suggestions.

Airfooox avatar Jul 25 '24 22:07 Airfooox

Well.. few months later, here I am.

I pushed two commits. One checks if the numbers are actually integers or not.

Second commit checks the step. I chose to check if the divider is an integer, is less than the upperLimit (for example 59 for seconds/minutes, 23 for hours, 31 for months) and if the step value is larger than the range. For example: 10-20/2 would be okay and give us 10, 12, 14, 16, 18, 20 10-20/10 would be okay and give us 10, 20 10-20/11 would be not okay and give us 10, 21, which is outside. So my check is if the second value is still within the range, otherwise the stepper operation would make such sense.

Is that a sensible change?

Airfooox avatar Dec 14 '24 12:12 Airfooox