Evaluation: Raise EvaluationOutOfRangeException if year 10k is hit during evaluation
#785
Conversation
Codecov ReportAttention: Patch coverage is
❌ Your project status has failed because the head coverage (66%) is below the target coverage (80%). You can increase the head coverage or adjust the target coverage. @@ Coverage Diff @@
## main #785 +/- ##
===================================
Coverage 66% 66%
===================================
Files 105 106 +1
Lines 4427 4464 +37
Branches 1076 1076
===================================
+ Hits 2922 2962 +40
+ Misses 1059 1057 -2
+ Partials 446 445 -1
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
minichma
force-pushed
the
work/minichma/feature/overflow_exception
branch
2 times, most recently
from
56d29b3 to
cf97c73
Compare
minichma
force-pushed
the
work/minichma/feature/overflow_exception
branch
3 times, most recently
from
7944167 to
882f8e6
Compare
|
@axunonb I introduced the new |
This is more generic for arithmetic operations, while |
| throw new Exception("FrequencyType.NONE cannot be evaluated. Please specify a FrequencyType before evaluating the recurrence."); | ||
| } | ||
| } | ||
| catch (ArgumentOutOfRangeException) |
There was a problem hiding this comment.
Sticking to the ical.net definitions, isn't is about the Occurrence of the Event here?
How about sth. like
catch (ArgumentOutOfRangeException)
{
// intentionally don't include the outer exception
throw new EvaluationOutOfRangeException(
$"""
Evaluation aborted: An event occurrence starting at {dt} plus the next interval exceeds the maximum supported date/time value.
This may be caused by a recurrence pattern with a missing or excessive COUNT, or an UNTIL date missing or beyond the supported range.
"""
);
}There was a problem hiding this comment.
Sticking to the ical.net definitions, isn't is about the Occurrence of the Event here?
Are you referring to 'recurrence' vs 'occurrence' in the the message in the default case? Actually I didn't touch that one, but to my understanding recurrences are occurrences that are produced by recurring elements, i.e. by RRULEs, right? Acutally this method is only used with RRULEs, so this part of the message could be ok. But the whole method is not well suited in this class. It should either be moved to a utility class or to RecurrencePatternEvaluator. Happen to have related work prepared in a different, WIP PR.
Will try to improve the message in the EvaluationOutOfRangeException
There was a problem hiding this comment.
Evaluation aborted: An event occurrence starting at {dt} plus the next interval exceeds the maximum supported date/time value. This may be caused by a recurrence pattern with a missing or excessive COUNT, or an UNTIL date missing or beyond the supported range.
Hm, this is also not fully precise. Actually there isn't necessarily an occurrence at dt. I feel that the actual value also doesn't add too much value. Usually the problem here is just that people try to iterate an unbounded RRULE to its end, which isn't possible. The actual value, where the overflow happens, seems to be less relevant. Will try to find something.
There was a problem hiding this comment.
Actually there isn't necessarily an occurrence
agree.
The only intention regarding the messages is to make people see themselves what caused the exception and what they can do about it, instead of opening a ticket. Any wording serving this goal is fine.
| case FrequencyType.Yearly: | ||
| dt = old.AddDays(-old.DayOfYear + 1).AddYears(interval); | ||
| break; | ||
| // FIXME: use a more specific exception. |
There was a problem hiding this comment.
If EvaluationException instead of EvaluationOutOfRangeException would suffice, we could fix this with e.g.:
default:
throw new EvaluationException(
$"""
Evaluation aborted: '{pattern.Frequency}' cannot be evaluated.
Specify a valid FrequencyType before evaluating the recurrence.
"""
);There was a problem hiding this comment.
Yes, absolutely. But didn't touch this one, so lets do it in a different PR please. Suggest to avoid multi-line messages though.
There was a problem hiding this comment.
As we're beyond our time budget all the time anyways, I don't appreciate extending the scope if not necessary. I know, my own review comments often suggest otherwise though.
| catch (ArgumentOutOfRangeException) | ||
| { | ||
| // intentionally don't include the outer exception | ||
| throw new EvaluationOutOfRangeException("Evaluation was aborted because an event's end time is out of range. This commonly happens if an event has an unbounded RRULE or a very long duration. Consider applying the .TakeWhile() operator on the returned sequence."); |
There was a problem hiding this comment.
See below: Maybe
throw new EvaluationException(
"""
Evaluation aborted: Calculating the end time of the event occurrence resulted in an out-of-range value.
This commonly happens if the event has an unbounded RRULE or an excessively long duration.
Consider applying the .TakeWhile() operator on the returned sequence to limit the evaluation.
"""
);There was a problem hiding this comment.
Nice, will improve.
…er the last recurrence has been returned.
minichma
force-pushed
the
work/minichma/feature/overflow_exception
branch
from
340051e to
c8963a5
Compare
|
|
No worries. I don't think there could easily be a 'too academic' ;-) Tried to improve the messages a little - should be good enough. |
| // itself are already done earlier, before doing the actual enumeration. | ||
| // Intentionally don't include the outer exception as this most likely is not a technical but a usage error. | ||
| .Catch<T, ArgumentOutOfRangeException>(_ => throw new EvaluationOutOfRangeException("The maximum supported date/time value has been exceeded while evaluating occurrences. This is likely to happen in case of unbounded RRULEs. Consider applying .TakeWhile() on the returned sequence.")) | ||
| .Catch<T, ArgumentOutOfRangeException>(_ => throw new EvaluationOutOfRangeException("An out-of-range value was encountered while evaluating occurrences. This commonly happens when trying to enumerate an unbounded RRULE to its end. Consider applying the .TakeWhile() operator.")) |
If recurrence evaluation produced an unrepresentable date/time, it used to throw an unspecific
ArgumentOutOfRangeExceptionorOverflowException. With this PR we throw a more specificEvaluationoutOfRangeException.As an overflow could happen virtually anywhere throughout the evaluation code, the overflow handling is implemented in a quite coarse manner. I.e. all
ArgumentOutOfRangeExceptions (raised byDateOnlyet al) as well asOverflowExceptions (raised by NodaTime) are caught in central places. They could theoretically also be caused for different reasons, but, as patterns are validated upfront, it should be quite safe to assume they are caused due to y10k overflows.Addresses #784