Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decimal values do not cause an error #291

Open
annybs opened this issue May 8, 2024 · 6 comments
Open

Decimal values do not cause an error #291

annybs opened this issue May 8, 2024 · 6 comments

Comments

@annybs
Copy link

annybs commented May 8, 2024

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?!

@Airfooox
Copy link
Owner

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

@ArtyomCZ
Copy link

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

image

@Airfooox
Copy link
Owner

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)?

@bombsimon
Copy link

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.

@invmatt
Copy link

invmatt commented Jun 27, 2024

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?

@Airfooox
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants