Make occurrences in chronological order even if BYMONTHDAY or BYYEARDAY is not in order (#666)

The recurrence iterator has a problem where it makes occurrences in the same order as given in
BYMONTHDAY or BYYEARDAY, which doesn't make sense - it should always return them in chronological
order.

This patch fixes it by pre-ordering the days that can be used when the month or year changes.
A day that is valid for one month/year may be different or not valid for the next month/year, so
the days need to be recomputed each time.

Author: darktrojan

lib/ical/recur_iterator.js

@@ -233,7 +233,7 @@ class RecurIterator {
     this.last.second = this.setup_defaults("BYSECOND", "SECONDLY", this.dtstart.second);
     this.last.minute = this.setup_defaults("BYMINUTE", "MINUTELY", this.dtstart.minute);
     this.last.hour = this.setup_defaults("BYHOUR", "HOURLY", this.dtstart.hour);
-    let dayOffset = this.last.day = this.setup_defaults("BYMONTHDAY", "DAILY", this.dtstart.day);
+    this.last.day = this.setup_defaults("BYMONTHDAY", "DAILY", this.dtstart.day);
     this.last.month = this.setup_defaults("BYMONTH", "MONTHLY", this.dtstart.month);
 
     if (this.rule.freq == "WEEKLY") {
@@ -262,77 +262,79 @@ class RecurIterator {
       this._nextByYearDay();
     }
 
-    if (this.rule.freq == "MONTHLY" && this.has_by_data("BYDAY")) {
-      let tempLast = null;
-      let initLast = this.last.clone();
-      let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
-
-      // Check every weekday in BYDAY with relative dow and pos.
-      for (let bydow of this.by_data.BYDAY) {
-        this.last = initLast.clone();
-        let [pos, dow] = this.ruleDayOfWeek(bydow);
-        let dayOfMonth = this.last.nthWeekDay(dow, pos);
-
-        // If |pos| >= 6, the byday is invalid for a monthly rule.
-        if (pos >= 6 || pos <= -6) {
-          throw new Error("Malformed values in BYDAY part");
-        }
+    if (this.rule.freq == "MONTHLY") {
+      if (this.has_by_data("BYDAY")) {
+        let tempLast = null;
+        let initLast = this.last.clone();
+        let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
+
+        // Check every weekday in BYDAY with relative dow and pos.
+        for (let bydow of this.by_data.BYDAY) {
+          this.last = initLast.clone();
+          let [pos, dow] = this.ruleDayOfWeek(bydow);
+          let dayOfMonth = this.last.nthWeekDay(dow, pos);
+
+          // If |pos| >= 6, the byday is invalid for a monthly rule.
+          if (pos >= 6 || pos <= -6) {
+            throw new Error("Malformed values in BYDAY part");
+          }
 
-        // If a Byday with pos=+/-5 is not in the current month it
-        // must be searched in the next months.
-        if (dayOfMonth > daysInMonth || dayOfMonth <= 0) {
-          // Skip if we have already found a "last" in this month.
-          if (tempLast && tempLast.month == initLast.month) {
-            continue;
+          // If a Byday with pos=+/-5 is not in the current month it
+          // must be searched in the next months.
+          if (dayOfMonth > daysInMonth || dayOfMonth <= 0) {
+            // Skip if we have already found a "last" in this month.
+            if (tempLast && tempLast.month == initLast.month) {
+              continue;
+            }
+            while (dayOfMonth > daysInMonth || dayOfMonth <= 0) {
+              this.increment_month();
+              daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
+              dayOfMonth = this.last.nthWeekDay(dow, pos);
+            }
           }
-          while (dayOfMonth > daysInMonth || dayOfMonth <= 0) {
-            this.increment_month();
-            daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
-            dayOfMonth = this.last.nthWeekDay(dow, pos);
+
+          this.last.day = dayOfMonth;
+          if (!tempLast || this.last.compare(tempLast) < 0) {
+            tempLast = this.last.clone();
           }
         }
-
-        this.last.day = dayOfMonth;
-        if (!tempLast || this.last.compare(tempLast) < 0) {
-          tempLast = this.last.clone();
+        this.last = tempLast.clone();
+
+        //XXX: This feels like a hack, but we need to initialize
+        //     the BYMONTHDAY case correctly and byDayAndMonthDay handles
+        //     this case. It accepts a special flag which will avoid incrementing
+        //     the initial value without the flag days that match the start time
+        //     would be missed.
+        if (this.has_by_data('BYMONTHDAY')) {
+          this._byDayAndMonthDay(true);
         }
-      }
-      this.last = tempLast.clone();
-
-      //XXX: This feels like a hack, but we need to initialize
-      //     the BYMONTHDAY case correctly and byDayAndMonthDay handles
-      //     this case. It accepts a special flag which will avoid incrementing
-      //     the initial value without the flag days that match the start time
-      //     would be missed.
-      if (this.has_by_data('BYMONTHDAY')) {
-        this._byDayAndMonthDay(true);
-      }
-
-      if (this.last.day > daysInMonth || this.last.day == 0) {
-        throw new Error("Malformed values in BYDAY part");
-      }
-    } else if (this.has_by_data("BYMONTHDAY")) {
-      // Attempting to access `this.last.day` will cause the date to be normalised.
-      // So it will never be a negative value or more than the number of days in the month.
-      // We keep the value in a separate variable instead.
 
-      // Change the day value so that normalisation won't change the month.
-      this.last.day = 1;
-      let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
+        if (this.last.day > daysInMonth || this.last.day == 0) {
+          throw new Error("Malformed values in BYDAY part");
+        }
+      } else if (this.has_by_data("BYMONTHDAY")) {
+        // Change the day value so that normalisation won't change the month.
+        this.last.day = 1;
 
-      if (dayOffset < 0) {
-        // A negative value represents days before the end of the month.
-        this.last.day = daysInMonth + dayOffset + 1;
-      } else if (this.by_data.BYMONTHDAY[0] > daysInMonth) {
-        // There's no occurrence in this month, find the next valid month.
-        // The longest possible sequence of skipped months is February-April-June,
-        // so we might need to call next_month up to three times.
-        if (!this.next_month() && !this.next_month() && !this.next_month()) {
-          throw new Error("No possible occurrences");
+        // Get a sorted list of days in the starting month that match the rule.
+        let normalized = this.normalizeByMonthDayRules(
+          this.last.year,
+          this.last.month,
+          this.rule.parts.BYMONTHDAY
+        ).filter(d => d >= this.last.day);
+
+        if (normalized.length) {
+          // There's at least one valid day, use it.
+          this.last.day = normalized[0];
+          this.by_data.BYMONTHDAY = normalized;
+        } else {
+          // There's no occurrence in this month, find the next valid month.
+          // The longest possible sequence of skipped months is February-April-June,
+          // so we might need to call next_month up to three times.
+          if (!this.next_month() && !this.next_month() && !this.next_month()) {
+            throw new Error("No possible occurrences");
+          }
         }
-      } else {
-        // Otherwise, reset the day.
-        this.last.day = dayOffset;
       }
     }
   }
@@ -513,7 +515,10 @@ class RecurIterator {
     let rule;
 
     for (; ruleIdx < len; ruleIdx++) {
-      rule = rules[ruleIdx];
+      rule = parseInt(rules[ruleIdx], 10);
+      if (isNaN(rule)) {
+        throw new Error('Invalid BYMONTHDAY value');
+      }
 
       // if this rule falls outside of given
       // month discard it.
@@ -747,6 +752,9 @@ class RecurIterator {
       if (this.by_indices.BYMONTHDAY >= this.by_data.BYMONTHDAY.length) {
         this.by_indices.BYMONTHDAY = 0;
         this.increment_month();
+        if (this.by_indices.BYMONTHDAY >= this.by_data.BYMONTHDAY.length) {
+          return 0;
+        }
       }
 
       let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
@@ -762,7 +770,6 @@ class RecurIterator {
       } else {
         this.last.day = day;
       }
-
     } else {
       this.increment_month();
       let daysInMonth = Time.daysInMonth(this.last.month, this.last.year);
@@ -835,7 +842,6 @@ class RecurIterator {
   }
 
   next_year() {
-
     if (this.next_hour() == 0) {
       return 0;
     }
@@ -844,6 +850,13 @@ class RecurIterator {
       this.days_index = 0;
       do {
         this.increment_year(this.rule.interval);
+        if (this.has_by_data("BYMONTHDAY")) {
+          this.by_data.BYMONTHDAY = this.normalizeByMonthDayRules(
+            this.last.year,
+            this.last.month,
+            this.rule.parts.BYMONTHDAY
+          );
+        }
         this.expand_year_days(this.last.year);
       } while (this.days.length == 0);
     }
@@ -854,19 +867,19 @@ class RecurIterator {
   }
 
   _nextByYearDay() {
-      let doy = this.days[this.days_index];
-      let year = this.last.year;
-      if (doy < 1) {
-          // Time.fromDayOfYear(doy, year) indexes relative to the
-          // start of the given year. That is different from the
-          // semantics of BYYEARDAY where negative indexes are an
-          // offset from the end of the given year.
-          doy += 1;
-          year += 1;
-      }
-      let next = Time.fromDayOfYear(doy, year);
-      this.last.day = next.day;
-      this.last.month = next.month;
+    let doy = this.days[this.days_index];
+    let year = this.last.year;
+    if (doy < 1) {
+        // Time.fromDayOfYear(doy, year) indexes relative to the
+        // start of the given year. That is different from the
+        // semantics of BYYEARDAY where negative indexes are an
+        // offset from the end of the given year.
+        doy += 1;
+        year += 1;
+    }
+    let next = Time.fromDayOfYear(doy, year);
+    this.last.day = next.day;
+    this.last.month = next.month;
   }
 
   /**
@@ -953,9 +966,19 @@ class RecurIterator {
         this.increment_year(years);
       }
     }
+
+    if (this.has_by_data("BYMONTHDAY")) {
+      this.by_data.BYMONTHDAY = this.normalizeByMonthDayRules(
+        this.last.year,
+        this.last.month,
+        this.rule.parts.BYMONTHDAY
+      );
+    }
   }
 
   increment_year(inc) {
+    // Don't jump into the next month if this.last is Feb 29.
+    this.last.day = 1;
     this.last.year += inc;
   }
 
@@ -1177,6 +1200,14 @@ class RecurIterator {
     } else {
       this.days = [];
     }
+
+    let daysInYear = Time.isLeapYear(aYear) ? 366 : 365;
+    this.days.sort((a, b) => {
+      if (a < 0) a += daysInYear + 1;
+      if (b < 0) b += daysInYear + 1;
+      return a - b;
+    });
+
     return 0;
   }