breejs / later

*Maintained fork of Later.* A javascript library for defining recurring schedules and calculating future (or past) occurrences for them. Includes support for using English phrases and Cron schedules. Works in Node and in the browser.
https://breejs.github.io/later/
MIT License
132 stars 12 forks source link

No parsing error when passing an invalid cron expression to `cron` function #7

Open naz opened 3 years ago

naz commented 3 years ago

refs https://github.com/bunkat/later/issues/174

Problem

As pointed out in referenced issue there is no error detection logic in later.parse.cron function. This introduced an inconsistency with how later.parse.text behave when invalid values are passed in. In case of cron method there is no indication of any type of error if completely invalid string is passed in. Contrary for text method there's error property assigned to returned object when the expression is not parsed.

Possible solution

There are multiple ways I can see this issue can be approached:

  1. Leave it as is, but be very specific that making sure the cron expression is valid is on the client
  2. Validate cron expression internally and return consistent error just like in case of parsing expression through text function

Personally I'm in favor of second approach as that would allow to unify error handling.

naz commented 3 years ago

To move on with the implementation I've ended up using cron-parser to validate cron expressions before calling later.parse.cron. Might serve as a good reference for parsing logic

naz commented 3 years ago

Had a look into what bree does with this exact problem and saw that it's running cron expression validation against cron-validate.

If the validation for cron expressions is moved into later's layer, I think it will make the logic for both cron and text expressions-to-schedule transformations more consistent.

hovancik commented 3 years ago

Just noticed the same issue. Would make sense to use same as for text parser as right now cron gives me 0 for error all the time and that complicates everything.

arossert commented 1 month ago

I also noticed now that later is not validating cron strings, is there a plan to add this or do I need to validate it manually?