Avoid infinite loop with invalid YEARLY recurrence rule #621
Conversation
Pull Request Test Coverage Report for Build 8741965240Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
leftmostcat
force-pushed
the
es6-invalid-rrule-fix
branch
from
7bd0af5
to
36a3f3e
Compare
test/recur_test.js
Outdated
| checkNoInstance({ | ||
| freq: 'DAILY', | ||
| parts: { | ||
| BYYEARDAY: [200] | ||
| } | ||
| }, 'BYYEARDAY may only appear in YEARLY rules'); |
There was a problem hiding this comment.
Spec says that The BYYEARDAY rule part MUST NOT be specified when the FREQ rule part is set to DAILY, WEEKLY, or MONTHLY., so this combination should normally be an error instead of just not producing instances.
I guess that brings up the bigger question of should we just ignore such errors and be lenient, or should we throw. What about using ICAL.design.strict to determine if it should throw or silently show no instances?
There was a problem hiding this comment.
The robustness principle suggests to me that we should only error if we reach a point where we simply can't interpret the file any longer. I'd be okay with using strict as a guard here if that makes sense to you. Probably the best thing to do in this instance is to favor the FREQ property and any invalid parts that follow are silently dropped, resulting in occurrences that follow whatever can be validly interpreted. Unfortunately, the spec provides rules for how to create valid iCalendar objects, but no real guidance on how to deal with invalid ones.
There was a problem hiding this comment.
Yeah, generally I'd like ical.js to be able to parse everything and be lenient. The strict mode was meant for the case where you might use ical.js as a validator and would want it to bail. Let's put this behind the strict mode check.
There was a problem hiding this comment.
I wasn't quite sure how to access whether we were in strict mode from the recurrence iter, so I've implemented the infinite loop prevention without modifying any throwing behavior. If that behavior is desire, it may be best to address that in a followup.
|
It looks like we haven't heard back on this issue, therefore we are closing this issue. If this problem persists in the latest version of ical.js, please re-open this issue. |
|
I've addressed the comments left. The timezones CI job seems to be failing due to something entirely unrelated that I don't understand. |
| @@ -1360,4 +1384,11 @@ class RecurIterator { | |||
| return result; | |||
| } | |||
| } | |||
|
|
|||
| class InvalidRecurrenceRuleError extends Error { | |||
| let iterator = recur.iterator(start); | ||
|
|
||
| if (options.noInstance) { | ||
| assert.equal(iterator.next(), null); |
There was a problem hiding this comment.
You could also assert that completed is set, while you are at it.
See https://phabricator.services.mozilla.com/D188118 and darktrojan#12.