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

Exception swallowed in "AndFieldValueGenerator" #423

Open
jamescosford opened this issue Apr 26, 2020 · 5 comments
Open

Exception swallowed in "AndFieldValueGenerator" #423

jamescosford opened this issue Apr 26, 2020 · 5 comments

Comments

@jamescosford
Copy link

In AndFieldValueGenerator, in the function

protected List<Integer> generateCandidatesNotIncludingIntervalExtremes(int start, int end)

there is a try/catch which just logs the exception and returns an empty list.

Catched expected exception while generating candidates
com.cronutils.model.time.generator.NoSuchValueException: null
	at com.cronutils.model.time.generator.AndFieldValueGenerator.generateNextValue(AndFieldValueGenerator.java:52)
	at com.cronutils.model.time.generator.AndFieldValueGenerator.generateCandidatesNotIncludingIntervalExtremes(AndFieldValueGenerator.java:84)
	at com.cronutils.model.time.generator.FieldValueGenerator.generateCandidates(FieldValueGenerator.java:61)
	at com.cronutils.model.time.ExecutionTimeBuilder.forHoursMatching(ExecutionTimeBuilder.java:64)
	at com.cronutils.model.time.ExecutionTime.forCron(ExecutionTime.java:53)

So I have a situation where a client generates cron expressions interactively using the parser, and expressions which are "valid" then generate completely unexpected results when used with the ExecutionTime class.

The exception is printed, but there is no way to guard against this behaviour because the exception is not propagated.

An example expression is 0 0 0-07,17-0 ? * SAT which is parsed as every hour between 0 and 7 and every hour between 17 and 0 at Saturday day.

If this is expression is not usable, it should not be accepted by the parser. If that's too hard, at least when an ExecutionTime is created using a Cron which it can't use, an exception should be generated so it is possible to validate against that, and keep junk out of the system.

@jmrozanec
Copy link
Owner

@jamescosford thank you for reporting this. May we ask you to contribute a PR with some tests that would reproduce the issue? Thanks!

@jamescosford
Copy link
Author

@jmrozanec it looks like things have moved on a little since the version that was causing my issue; now I don't see an exception logged but I still get strange results from the scheduler. I will create a PR with a test around this issue.

@jmrozanec
Copy link
Owner

@jamescosford glad to hear that! That things moved on and the exception no longer appears, as well that you sent a PR 😄 Thanks! We will merge soon. Best!

@IndeedSi
Copy link

IndeedSi commented Oct 12, 2020

I did some digging into the code. The issue is that even though the parser allows overflowing ranges 17-0 in #396 , but these overflowing ranges are not taking effect in ExecutionTime. So for the example expression, only 0-7 will be considered, 17-0 will be ignored

I feel this could be a rather larger change than expected. As in quartz, the expression needs to be overflowed to the next hour / day / month..

@jmrozanec
Copy link
Owner

@IndeedSi thank you for the insight!

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