Bug in steps calculation

[afsalthaj]

The calculation of prev execution from the current time does seem to return None when
the current time is 1st of every month and previous month is less than 31 days.

This happens regardless of whether you pass LocalDate or LocalDateTime
ex: If the cron expression is val cron = cron"0 0 0 * * ?" (daily) , then cron.prev(LocalDate.of(2019, 3, 1).atStartOfDay) returns None (Feb has less than 31 days) and cron.prev(LocalDate.of(2019, 4, 1).atStartOfDay) returns 31st of March.

[alonsodomin]

Hi @afsalthaj, thanks for reporting this. This has been under the radar for a while so nice to have documented here.

Fixing this is kinda hard with current internals and kinda feel that offering the prev operation was a mitake. I started work in a branch to revisit the AST and try to address some of the limitations that the library has in regards to supporting complex cron expressions.

I have a hunch that the revamping of the internals could help fixing this issue but I'm also considering dropping it altogether if it doesn't. If you think you could provide a quick fix then a PR would be more than welcome.

[afsalthaj]

@alonsodomin Thanks for the response. I am yet to dig into the details of the internals of cron4s. I will take a look, and see how it goes. I am with you on considering dropping the prev function if we are not able to fix it.

[alonsodomin]

great, let me know if you need some help with anything, we could chat via the gitter channel

[dave-handy]

Spent about 2 hours looking at this problem before finding the bug. If this isn't fixed soon I'd recommend adding it to the scaladoc on prev or removing the prev call.

The stepping code is a bit over my head but I may have a coworker that can help. The way I set up my user code (already merged before running into this) it is only using prev, so I'd love for this to get fixed so I don't have to work around it some strange way.

[alonsodomin]

My appologies for the time wasted due to this bug, it was a naive move from my side to include a prev operation as in some simple expressions it looked like a similar algebra could be used for both next and prev operations and I found it useful to solve a particular problem I had when I started this library.

The reality is that the number of corner cases when going backwards in time is much bigger than when moving forwards and hence with current stepping code the problem is not solvable.

At this moment I can't give any timeframes on fixing this and I've been considering removing it for good and potentially add it later if the new stepping algo can solve it (which I is yet to be proven). Adding it to the scaladoc doesn't seem like a good option since that would mean documenting in which cases the operation returns a valid result and which ones fail.

The problem is not impossible to solve, just very tedious, but I would not recommend to get entangled into current implementation to find a solution since it requires a redesign of some of the internals. Happy to give a hand with it if this operation is so important for you guys.

[afsalthaj]

I tried looking at it, and it was taking a bit of time to make changes. Getting this operation into working state would be nice for the library, as it does other operations pretty elegantly.

[alonsodomin]

I'll take a look over this week. Mind if I ask you what is your use case?

[dave-handy]

In my case, I am executing some analytics on a schedule and storing them in a database. Calculating the most recent (previous) scheduled execution time allows the aggregation to populate in the DB anytime after the execution time has elapsed, in case the service is down for an upgrade or the operation otherwise fails. In the meantime I've added code to try to calculate the most likely previous invocation after a date in the past rather than using previous relative to "now".

TLDR; the cron is used to calculate the start and end time windows for the analytic. The end of the range is always in the past, so using "previous" is a bit more intuitive.

[alonsodomin]

Thanks @dave-handy, I see your point, your workaround is probably your best bet right now but agree that counter-intuitive and suboptimal.

I do see value in having a prev operation and that's why I've been reticent of removing it. I'll try to steam up and attempt to fix it with the new approach.

[chuwy]

Just two arbitrary cents more in defence of prev operation - it's a crucial part of our app. We have an ability to set windows like: {"when": "0 15 10 * * ?", "duration": "1 hour"} and if the app starts within such window it must know about it to make appropriate actions - otherwise the window is not recognized until a next match.

Even if the behavior was corner-case-ridden it's stil very useful for such use case in lieu of other solutions (which I might just miss).

[alonsodomin]

that's actually valuable feedback, and a good reason to not remove it