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

Incorrect date generation #81

Closed
janostik opened this issue Apr 21, 2016 · 7 comments
Closed

Incorrect date generation #81

janostik opened this issue Apr 21, 2016 · 7 comments

Comments

@janostik
Copy link
Contributor

Cron utils incorrectly maps MON,TUE,.. to 2,3,4 in Quartz cron type. e.g. MON is mapped to 1, which is Sunday in Quartz system. Test to reproduce:

`@Test
public void test_weekday_number_name_matching() {
CronDefinition cronDefinition = CronDefinitionBuilder.instanceDefinitionFor(CronType.QUARTZ);
CronParser parser = new CronParser(cronDefinition);

    DateTime fridayMorning = new DateTime(2016, 4, 22, 0, 0, 0, DateTimeZone.UTC);
    ExecutionTime numberExec = ExecutionTime.forCron(parser.parse("0 0 12 ? * 2,3,4,5,6 *"));
    ExecutionTime nameExec = ExecutionTime.forCron(parser.parse("0 0 12 ? * MON,TUE,WED,THU,FRI *"));

    assertEquals("Assert same generated dates",
            numberExec.nextExecution(fridayMorning),
            nameExec.nextExecution(fridayMorning));
}`

date generated form MON,TUE,WED,.. expression : 2016-04-24T12:00:00.000Z (Sunday)

@jmrozanec
Copy link
Owner

@janostik thank you for reporting this. Won't you mind to contribute the test above as a pull request? Patches are welcome as well :)

@janostik
Copy link
Contributor Author

sure thing! I'll do the PR and look for possible patch later this week.

@jmrozanec
Copy link
Owner

@janostik Great! Thanks!

@jmrozanec
Copy link
Owner

jmrozanec commented May 13, 2016

Partially solved with PR #84. Kudos to @janostik for contributing a fix!
We still need to analyze how to deal with a specific range case.

jmrozanec added a commit that referenced this issue May 17, 2016
@jmrozanec
Copy link
Owner

jmrozanec commented May 17, 2016

@janostik below I provide some more context on fixing the issue about currently not supported "between" ranges, as mentioned in #84
"Between" element describes a range, and every range should start with an element which is less or equal than the element that the end of the range. I enforced this rule into the object, but cron expressions admit exceptions:

Both cases illustrate that more intelligence is required at parse time:

  • the first case can be simplified as a union of elements (in this case: AND(ON(SAT), ON(SUN))
  • the second case, if L-3 is specified as DoM, must be understood as 3-L, otherwise would be an empty range.

This may require a refactor on how do we parse, represent and validate this piece of cron expression.

If interested, I invite you to discuss ideas and work together on them on a separate branch, in order to fix this and related issues. By decoupling representation, from validations, we may get greater flexibility and perform more complex validations (ex.: question mark support at Quartz: requires to be present at either DoM or DoW, and we cannot support this semantics yet).

@jmrozanec
Copy link
Owner

Opening a new issue for issues regarding range parsing: #86

@jmrozanec
Copy link
Owner

@janostik New version released! Please check details here

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

No branches or pull requests

2 participants