GetOccurrences should not assume the system local date time

[MartinRothschink]

RecurrenceUtil uses AsSystemLocal when taking in an IDateTime. (line 18) That's bad, because it loses information, and we have NodaTime backing the search, which should make working with time zones easy. In order, these should be the rules for time zone ambiguity:

1. The CalDateTime's tzid, if specified
2. Otherwise the Event's tzid, if specified
3. Otherwise the local system's time zone

Original ticket:

   /// <summary>
      /// New zealand time zone should return occurrence but fails for tomorrow.
      /// </summary>
      [Test]
      public void NewZealandTimeZoneShouldReturnOneOccurrenceForTomorrow()
      {
         const string ical =
     @"BEGIN:VCALENDAR
PRODID:-//AxoNet Software GmbH//NONSGML Lights-Out//EN
VERSION:2.0
CALSCALE:GREGORIAN
METHOD:PUBLISH
BEGIN:VTIMEZONE
TZID:New Zealand Standard Time
BEGIN:STANDARD
TZNAME:New Zealand Standard Time
TZOFFSETTO:+1200
TZOFFSETFROM:+1300
RRULE:FREQ=YEARLY;BYDAY=SU;BYMONTH=4;BYSETPOS=1;WKST=MO
DTSTART:16010101T030000
END:STANDARD
BEGIN:DAYLIGHT
TZOFFSETFROM:+1200
TZOFFSETTO:+1300
DTSTART:20080101T020000
RRULE:FREQ=YEARLY;BYDAY=SU;BYMONTH=9;BYSETPOS=-1;WKST=MO
TZNAME:New Zealand Daylight Time
END:DAYLIGHT
END:VTIMEZONE
BEGIN:VEVENT
DTSTART;TZID=New Zealand Standard Time:20161212T164500
DTEND;TZID=New Zealand Standard Time:20161212T210002
SUMMARY:Server
DESCRIPTION:
UID:36a33ca8-92ba-49bc-a26a-712873030788
X-MICROSOFT-CDO-BUSYSTATUS:BUSY
BACKGROUND:BUSY
RRULE:FREQ=DAILY
END:VEVENT
END:VCALENDAR
";

         var collection = Calendar.LoadFromStream(new StringReader(ical));
         var checkAgainst = new DateTime(2016, 12, 13);
         var occurrences = collection.GetOccurrences<Event>(checkAgainst);

         Assert.IsTrue(occurrences.Count.Equals(1));
      }

Using ical.net 2.2.18.

This is a simple daily recurrence starting today (12/12). Therefore I expect to get one occurrence for each day (today, tomorrow, the day after tomorrow, etc).

It works for other time zones but fails for New Zealand TZ (returns no occurrence after today).

[rianjs]

It looks like there's a bad assumption encoded in RecurrenceUtil.GetOccurrences on line 18:

public static HashSet<Occurrence> GetOccurrences(IRecurrable recurrable, IDateTime dt, bool includeReferenceDateInResults)
{
    return GetOccurrences(recurrable, new CalDateTime(dt.AsSystemLocal.Date), new CalDateTime(dt.AsSystemLocal.Date.AddDays(1).AddSeconds(-1)),
        includeReferenceDateInResults);
}

AsSystemLocal will have inconsistent behavior with respect to many things, especially in scenarios where your local timezone differs from the time zone of your server (for example). It's also not cross-platform, which is bad. Really it should respect the timezone id that's provided in the Event, or if you were to give it a CalDateTime instead of a DateTime, use that tzid instead.

I think in order, the rules should be to prefer:

1. The CalDateTime's tzid, if specified
2. Otherwise the Event's tzid, if specified
3. Otherwise the local system's time zone

Maybe what we really need is a type that represents DateTime + IANA/BCL time zone and restrict the usage of GetOccurrences to that type to minimize ambiguity.

[MartinRothschink]

Thanks for the explanation. Any chance to get a fix/workaround? That's currently a showstopper for my NZ users.

[rianjs]

The easiest workaround is to widen the search range to -1 and +1 days so you're always searching for a 3 day range. You could then filter that result set pretty easily. I tried creating a CalDateTime with tz specified, and that wasn't sufficient.

My time and energy has been limited, and all the people that were making commits and PRs this summer have gone elsewhere... so I'm not sure when it will get fixed.

I'll recharacterize your ticket so the root cause (or what I suspect is the root cause) is easier to understand.

[MartinRothschink]

Thanks will give it a try. BTW. I changed the system time zone to NZ and it's still not working.

[MartinRothschink]

Doing

var checkAgainst = new DateTime(2016, 12, 12);
var occurrences = collection.GetOccurrences<Event>(checkAgainst.AddDays(-1), checkAgainst.AddDays(1));

Returns 1 event for December 11, not 12! That's getting pretty difficult.

What I still not understand: Why does it also happen if I set my local time zone to NZ? In that case AsSystemLocal should be the same as Value?

[MartinRothschink]

I had a look at RecurrenceUtil.cs. Line 18 is never called here, it is 24-52. The problem seems to start here:

// Change the time zone of periodStart/periodEnd as needed 
// so they can be used during the evaluation process.
periodStart = DateUtil.MatchTimeZone(start, periodStart);
periodEnd = DateUtil.MatchTimeZone(start, periodEnd);

periodStart/End is no longer 00:00-23:59, it's now 11:00-10:59 for NZ.
The evaluator returns one event in periods:

var periods = evaluator.Evaluate(start, DateUtil.GetSimpleDateTimeData(periodStart), DateUtil.GetSimpleDateTimeData(periodEnd), includeReferenceDateInResults);

and then the filter drops the one found and returns null:

var otherOccurrences =
              from p in periods
              let endTime = p.EndTime ?? p.StartTime
              where endTime.GreaterThan(periodStart) && p.StartTime.LessThanOrEqual(periodEnd)
              select new Occurrence(recurrable, p);

otherOccurrences is empty.

[MartinRothschink]

It all starts in calendar.cs at line 351. AsSystemLocal is not used.

 public virtual HashSet<Occurrence> GetOccurrences(DateTime dt)
 {
     return GetOccurrences<IRecurringComponent>(new CalDateTime(dt.Date), new CalDateTime(dt.Date.AddDays(1).AddSeconds(-1)));
 }

[MartinRothschink]

If I comment these two lines, it seems to work and all unit tests are still successful:

periodStart = DateUtil.MatchTimeZone(start, periodStart);
periodEnd = DateUtil.MatchTimeZone(start, periodEnd);

[ColinNg]

@MartinRothschink I tried commenting out the 2 lines you mentioned - the DocumentationExamples.cs > Daily_Test() still fails for me. I'm in Americas/Vancouver GMT -8 or -7 depending on Daylight Saving Time.

I started debugging but I realize the source of my problem may be different from yours. I'll fix the one for my workplace first and then I'll run the test as you've provided after.

In RecurrencePatternEvaluator.cs > ProcessRecurrencePattern(...) there is this code:
r.Until = DateUtil.MatchTimeZone(referenceDate, new CalDateTime(r.Until)).Value;
The call to new CalDateTime(r.Until)) unfortunately assumes all input dates are Utc. This isn't true as in my case, it's America/Los Angeles (Local).

I think the 'proper' solution would be to change the Until to require a CalDateTime input, then its time zone is specified. But I don't know what impact it will have on the rest of the code yet. So I will chicken out and make an override of public static IDateTime MatchTimeZone(IDateTime dt1, IDateTime dt2) that accepts a DateTime as the second argument. If it fixes the test, I might just leave it as-is (instead of refactoring the Until parameter, as I don't know the intent of having Until as DateTime whereas all other inputs are CalDateTime).

[rianjs]

@ColinNg and @MartinRothschink - Can you try version 4.0.1 from nuget? I did a bunch of work that should resolve this.