Conflicting Recurrence Rules and Exception Rules run indefinitely

[magiluke]

If I have conflicting rules and call next_occurrences, it will loop indefinitely, regardless of end_time.

For instance:

schedule = Schedule.new("2012-12-1".to_time, :end_time => "2012-12-10".to_time)
schedule.add_recurrence_rule Rule.daily
schedule.add_exception_rule Rule.daily
schedule.next_occurrences(20)

This will just loop on forever. In the next_time function, it will always find a new value:

if res = rule.next_time(time, self, closing_time)
  if min_time.nil? || res < min_time
    min_time = res
  end
end

And then break out of the loop because it finds an occurrence:

if exception_time?(min_time)
  time = min_time + 1
  min_time = nil
  next
end

It should consider the end date for when it hits a scenario like this, and perhaps have a max loop iteration limit when it finds no match, or at least an arbitrarily far off end date if one isn't supplied.

[avit]

Also, we could probably reject recurrence rules that are completely masked by exception rules before even processing them. But I wonder if it's worth it...

How do you arrive at the situation you describe, with the same rule in both rrules & exrules? Why should this be valid?

[magiluke]

I'd arrive there by having a user select a combination of rules that creates it when they are creating a schedule. And this should not be valid, but IceCube should be able to handle this situation.

[avit]

@seejohnrun, @forrest, Did you have any more progress with the approach from pull request #112? Agreed on your comment that it should be more functional-style, but I also think we can avoid looping over masked rules unnecessarily here.

[forrest]

I haven't anything with this since #112. However, my plan when I magically found free time is to split the next_time method into a dedicated object. Some kind of NextTimeCalculator class which would keep all the variables isolated, and cleanup how we pass the variables to the different layers. Will also allow us to write very focused unit specs for the model to help keep track of all the complex functionality getting added to the next_time.

As for avoiding un-necassary loops, there is a performance comment about sorting the rules first (https://github.com/seejohnrun/ice_cube/blob/master/lib/ice_cube/validated_rule.rb#L92-L98) Perhaps, once sorted there is a simple way to detect permanently masked rules and raise a useful UnresolvableRuleException when permanently masked?

[avit]

Gotcha, that makes sense. I was first thinking of a simpler solution using ruby's "retry" in the next_time method to avoid dealing with the instance variable state, but extracting to a class is a better goal.

About raising UnresolvableRuleException, I wonder if the rules should be validated defensively because the exception could be far removed from the point when the rules are added:

1. Create a schedule with a combination of rules
2. Serialize & persist to database
3. Reload schedule for displaying in public view
4. Exception happens when trying to display next occurrences

I'm thinking that exception should be raised & handled before serializing to database or ical, or any access to occurrences.

[avit]

EXRULE is deprecated, IceCube should follow suit with that.

Still, it's possible to create impossible combinations such as:

Rule.daily.day_of_month(30).month_of_year(2)
Rule.daily.day_of_month(30).day_of_week(:monday => [1])

I will add checks for conflicts when adding rules: it's better to throw an exception than to cause DOS from an infinite loop.

[avit]

EXRULEs have been deprecated and removed from serialization since 0.11, which means that the schedule could behave differently on first setup vs. serialize and reload... I'm considering we restore the serialization or make this more consistent, perhaps with an error.

Rules of same type

schedule.rrules = [Rule.daily.month_of_year(11, 12, 1, 2, 3)]
schedule.exrules = [Rule.daily.month_of_year(1)]

This could easily strip out the matching exception rule and collapse it onto the RRULE without an error. It might be easier to just throw an error, especially if there are validations on these rules then they would have to match exactly.

Rules of different types

These could just be allowed to serialize: I don't see iCalendar RFC getting updated any time soon so the deprecation of EXRULEs is mostly advisory at this point. Being different types, they would not overlap 100% so that nothing is found.

Thoughts?