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

A cron-utils bug by withValidRange #428

Closed
HongZhaoHua opened this issue May 11, 2020 · 8 comments · Fixed by #454
Closed

A cron-utils bug by withValidRange #428

HongZhaoHua opened this issue May 11, 2020 · 8 comments · Fixed by #454

Comments

@HongZhaoHua
Copy link

define CronDefinitionBuilder.instanceDefinitionFor(CronType.QUARTZ)

use cron "0 0 12 * * ?"

and use dateTime = ZonedDateTime.of(1900, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);

now, run execution.nextExecution(dateTime);

you will get the result is 1900-1-1T12:00

but in fact, the result must be 1970-1-1T12:00 because CronType.QUARTZ was limit the year range from 1970 to 2099.

@HongZhaoHua
Copy link
Author

here is the unit test. plz check it.

@Test
public void testBoundary() {
    CronDefinition cronDefinition = CronDefinitionBuilder.instanceDefinitionFor(CronType.QUARTZ);
    CronParser parser = new CronParser(cronDefinition);
    ExecutionTime execution = ExecutionTime.forCron(parser.parse("0 0 12 * * ?"));
    {
        ZonedDateTime dateTime = ZonedDateTime.of(1900, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
        dateTime = execution.nextExecution(dateTime).orElse(null);
        Assert.assertEquals(ZonedDateTime.of(1970, 1, 1, 12, 0, 0, 0, ZoneOffset.UTC), dateTime);
    }
    {
        ZonedDateTime dateTime = ZonedDateTime.of(2150, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC);
        dateTime = execution.lastExecution(dateTime).orElse(null);
        Assert.assertEquals(ZonedDateTime.of(2099, 12, 31, 12, 0, 0, 0, ZoneOffset.UTC), dateTime);
    }
}

@HongZhaoHua
Copy link
Author

version is 9.0.2.

@jmrozanec
Copy link
Owner

jmrozanec commented May 11, 2020

@HongZhaoHua thanks! If it is not too much to ask, may you send us a PR with the proposed test? Thanks!

@HongZhaoHua
Copy link
Author

@jmrozanec
The proposed test was on the 2th floor, don't u see it?
^^

@jmrozanec
Copy link
Owner

jmrozanec commented Jul 11, 2020

@HongZhaoHua sure we see it 😄 But on GitHub we usually contribute code as PR and not through comments. If you have some minutes and could be so kind as to post a PR, we will be glad to merge it. Thanks!

@IndeedSi
Copy link

I'm wondering if this is really a bug. It is true that QUARTZ cron expression only takes year between 1970 and 2099, but do we have that limit for the execution time?
"0 0 12 * * ?" means a job which fires at 12:00:00pm every day. I don't see how this expression limits the fire dates to be within 1970, 2099.

@jmrozanec
Copy link
Owner

@IndeedSi we shall support the definition, including the constraints.

@IndeedSi
Copy link

@jmrozanec Gotcha. I have got a change for passing the constraint from cron definition to execution time.

jmrozanec added a commit that referenced this issue Oct 13, 2020
…ution-time

#428 Pass valid range constraints from cron definition to execution time
@jmrozanec jmrozanec added this to the next milestone Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants