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

Handling a a logical inconsistency RRULE #137

Open
chungonn opened this issue May 18, 2024 · 5 comments
Open

Handling a a logical inconsistency RRULE #137

chungonn opened this issue May 18, 2024 · 5 comments

Comments

@chungonn
Copy link

Hi,

First, I would like to thank you for nice new API for creating recurrenceSet, it is very intuitive and flexible. Though there are breaking change from 0.15.0 to 0.17.1, the improvements in the library are worthy of the upgrade.

This issue existed since 0.15.0. Given an illogical rrule caused by BYPOS=2
"FREQ=WEEKLY;INTERVAL=2;BYDAY=SA;BYHOUR=19;BYMINUTE=0;BYSECOND=0;BYSETPOS=2"

The RecurrenceSet#hasNext or RecurrenceSet#next will throw the following exception below. In this situation, should the InstanceIterator a false or null instead?

java.lang.IllegalStateException: too many empty recurrence sets

at org.dmfs.rfc5545.recur.BySetPosFilter.nextSet(BySetPosFilter.java:88)
at org.dmfs.rfc5545.recur.BySetPosFilter.next(BySetPosFilter.java:69)
at org.dmfs.rfc5545.recur.RecurrenceRuleIterator.fetchNextInstance(RecurrenceRuleIterator.java:95)
at org.dmfs.rfc5545.recur.RecurrenceRuleIterator.<init>(RecurrenceRuleIterator.java:89)
at org.dmfs.rfc5545.recur.RecurrenceRule.iterator(RecurrenceRule.java:2218)
at org.dmfs.rfc5545.recurrenceset.OfRule.iterator(OfRule.java:42)
at org.dmfs.jems2.iterator.Mapped.next(Mapped.java:64)
@dmfs
Copy link
Owner

dmfs commented May 18, 2024

I'm glad to hear that you enjoy the new API.

I've spent quite some time thinking about this issue. Unfortunately it's not trivial to separate rules that never emit an occurrence from those that do so every couple of thousand occurrences (see #48 for an example of that).
In case of BYSETPOS we'd have to decide whether the rule ever has that many instance in an interval. That alone can be a challenging task. Just think of something like

FREQ=WEEKLY;BYMONTH=2;BYMONTHDAY=15,29;BYDAY=SA;BYSETPOS=2

This rule doesn't make much sense and certainly can be simplified. However, the rule is valid. Yet only every 28 years / ~1450 weeks there is a February in which the 29th falls on a Saturday. You can easily modify it to never emit anything.

I see three possible solutions right now:

  1. keep the code as it is and live with those edge cases not being treated properly
  2. throw a custom (Runtime-) Exception instead of a generic one, so the caller can at least (reliably) distinguish that case from another IlleagalStateException that might occur
  3. Implement some code to recognize (at least some of) those edge cases and stop iterating without an error when no results are to be expected.

I wonder if the implementation effort and the runtime overhead are worth implementing number 3.

@dmfs
Copy link
Owner

dmfs commented May 18, 2024

I've added #138 to cover option 2.

@chungonn
Copy link
Author

Hi Marten,

Thanks for sharing the challenges and your thoughts about the issue.

The first thing I thought when I had the problem was, wouldn't it be nice if there is a RecurrenceRule#isValid. And there is such a method but it is in private. It however checks for the well-formed-ness of the RRule which did not handle the logic error in my case. If there is such method to check both well-formed and logical error than the onus is on the user of the library.

I do agree with you that there are always so many things to do and we must put our energy and time on what provide the best return.

If I may suggest a simpler solution to my problem. If there is a logical error in the RRule then RecurenceSet#iterator#hasNext will return false as there is nothing to return correspondingly for RecurrenceSe#iterator#next will return null. Perhaps there is an addition state to hold the error if any for diagnostic purposes.

With this, even with an illogical RRule, the user will not get any errors but instead get a null for next and a false for hasNext especially in runtime, maybe frowned upon by some of the library users though,

@dmfs
Copy link
Owner

dmfs commented May 21, 2024

Hmm, the suggested behavior would violate the Iterator Contract. I'd rather not do that.

@chungonn
Copy link
Author

chungonn commented May 22, 2024 via email

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

No branches or pull requests

2 participants