jmrozanec/cron-utils

ClassCastException when calling ExecutionTime.forCron() for RebootCron

J0B10 opened this issue · 8 comments

J0B10 commented

Bug description

cron-utils Version: 307990d

When calling ExecutionTime.forCron(cron) on a Reboot schedule we get the following ClassCastException:

java.lang.ClassCastException: class com.cronutils.model.RebootCron cannot be cast to class 
com.cronutils.model.CompositeCron (com.cronutils.model.RebootCron and com.cronutils.model.CompositeCron are in unnamed
module of loader 'app')

	at com.cronutils.model.time.ExecutionTime.forCron(ExecutionTime.java:76)
	at org.betonquest.betonquest.api.schedule.CronSchedule.<init>(CronSchedule.java:91)
	at org.betonquest.betonquest.api.schedule.CronRebootScheduleTest$1.<init>(CronRebootScheduleTest.java:21)
	at org.betonquest.betonquest.api.schedule.CronRebootScheduleTest.createSchedule(CronRebootScheduleTest.java:21)

This exception is not thelling the library user what went wrong here.

Intended behaviour

I would assume either one of the following two behaviours, as clearly there is no way for the model to tell a next execution time for the reboot cron:

  1. ) Return a Execution time instance that does only return empty optionals for next & last execution time.
  2. ) Throw a checked exception that is more descriptive of the actual issue.

I personally prefer the 2nd option as I think it works better with existing code.

Related issues

#520

jmrozanec commented

@J0B10, may we ask you for a PR with a test for this issue? We would appreciate that 😄

J0B10 commented

I'm already working on it 😅

jmrozanec commented

@J0B10 I think we have a fix for it, so just the test would be enough 😄 Thank you for reporting! 🚀

  • 👍1
J0B10 commented

Uhh that was quick .👍 I first had to commit my code to our repo and open a PR there.
I'm doing way too much stuff at the same time. 😅

But yes, I'll write a test. The fix LGTM and is exactly what I envisioned.

What I still wonder if the CronDiscriptor or the CronMapper could have similar issues with the RebootCron.
I have not tested that though as we do not use these.

jmrozanec commented

@J0B10, we would appreciate it if you could add some tests for that too 😄 And then we see if works as expected 😄

jmrozanec commented

@J0B10 we implemented the fixes! 😎 A new release will follow soon 🚀 Thank you for contributing to cron-utils! 😄

  • ❤️1
jmrozanec commented

@J0B10 cron-utils version 9.2.0 was released with this functionality and the fixes for the issues above. Thank you again! 😄 🚀

  • ❤️1
J0B10 commented

Thanks a lot! We already updated our codebase to use the new release 🚀